From 808759a84bbdc8ac70e67c2b0409bf128df2048c Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 29 Sep 2024 14:02:25 +0200 Subject: [PATCH 01/20] Add support for undo in the viewer --- crates/viewer/re_ui/src/command.rs | 17 ++- crates/viewer/re_viewer/src/app.rs | 55 +++++++- crates/viewer/re_viewer/src/app_state.rs | 33 +++-- crates/viewer/re_viewer/src/lib.rs | 3 + crates/viewer/re_viewer/src/ui/rerun_menu.rs | 3 + crates/viewer/re_viewer/src/undo.rs | 120 ++++++++++++++++++ .../re_viewer_context/src/command_sender.rs | 7 + .../re_viewer_context/src/test_context.rs | 2 + 8 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 crates/viewer/re_viewer/src/undo.rs diff --git a/crates/viewer/re_ui/src/command.rs b/crates/viewer/re_ui/src/command.rs index c58b06069dc2..21109f45cc7f 100644 --- a/crates/viewer/re_ui/src/command.rs +++ b/crates/viewer/re_ui/src/command.rs @@ -21,6 +21,9 @@ pub enum UICommand { CloseCurrentRecording, CloseAllRecordings, + Undo, + Redo, + #[cfg(not(target_arch = "wasm32"))] Quit, @@ -120,7 +123,10 @@ impl UICommand { ), Self::CloseAllRecordings => ("Close all recordings", - "Close all open current recording (unsaved data will be lost)",), + "Close all open current recording (unsaved data will be lost)"), + + Self::Undo => ("Undo", "Undo the last blueprint edit for the open recording"), + Self::Redo => ("Redo", "Redo the last undone thing"), #[cfg(not(target_arch = "wasm32"))] Self::Quit => ("Quit", "Close the Rerun Viewer"), @@ -269,15 +275,15 @@ impl UICommand { } fn cmd_shift(key: Key) -> KeyboardShortcut { - KeyboardShortcut::new(Modifiers::COMMAND.plus(Modifiers::SHIFT), key) + KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, key) } fn cmd_alt(key: Key) -> KeyboardShortcut { - KeyboardShortcut::new(Modifiers::COMMAND.plus(Modifiers::ALT), key) + KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::ALT, key) } fn ctrl_shift(key: Key) -> KeyboardShortcut { - KeyboardShortcut::new(Modifiers::CTRL.plus(Modifiers::SHIFT), key) + KeyboardShortcut::new(Modifiers::CTRL | Modifiers::SHIFT, key) } match self { @@ -289,6 +295,9 @@ impl UICommand { Self::CloseCurrentRecording => None, Self::CloseAllRecordings => None, + Self::Undo => Some(cmd(Key::Z)), + Self::Redo => Some(cmd_shift(Key::Z)), + #[cfg(all(not(target_arch = "wasm32"), target_os = "windows"))] Self::Quit => Some(KeyboardShortcut::new(Modifiers::ALT, Key::F4)), diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index 086d5db78154..d76eacc9fddd 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -15,11 +15,11 @@ use re_viewer_context::{ SystemCommandSender, }; -use crate::app_blueprint::PanelStateOverrides; use crate::{ app_blueprint::AppBlueprint, app_state::WelcomeScreenState, background_tasks::BackgroundTasks, AppState, }; +use crate::{app_blueprint::PanelStateOverrides, BlueprintUndoState}; // ---------------------------------------------------------------------------- @@ -520,7 +520,8 @@ impl App { if self.state.app_options.inspect_blueprint_timeline { // We may we viewing a historical blueprint, and doing an edit based on that. // We therefor throw away everything after the currently viewed time (like an undo) - let last_kept_event_time = self.state.blueprint_query_for_viewer().at(); + let last_kept_event_time = + self.state.blueprint_query_for_viewer(blueprint_db).at(); let first_dropped_event_time = last_kept_event_time.inc(); blueprint_db.drop_time_range( &re_viewer_context::blueprint_timeline(), @@ -529,6 +530,14 @@ impl App { re_chunk::TimeInt::MAX, ), ); + } else { + let undo_state = self + .state + .blueprint_undo_state + .entry(blueprint_id) + .or_default(); + + undo_state.clear_redo(blueprint_db); } for chunk in updates { @@ -544,6 +553,23 @@ impl App { let mut time_ctrl = self.state.blueprint_cfg.time_ctrl.write(); time_ctrl.set_play_state(blueprint_db.times_per_timeline(), PlayState::Following); } + SystemCommand::UndoBlueprint { blueprint_id } => { + let blueprint_db = store_hub.entity_db_mut(&blueprint_id); + self.state + .blueprint_undo_state + .entry(blueprint_id) + .or_default() + .undo(blueprint_db); + } + SystemCommand::RedoBlueprint { blueprint_id } => { + let blueprint_db = store_hub.entity_db_mut(&blueprint_id); + self.state + .blueprint_undo_state + .entry(blueprint_id) + .or_default() + .redo(blueprint_db); + } + SystemCommand::DropEntity(blueprint_id, entity_path) => { let blueprint_db = store_hub.entity_db_mut(&blueprint_id); blueprint_db.drop_entity_path_recursive(&entity_path); @@ -710,6 +736,21 @@ impl App { .send_system(SystemCommand::CloseAllRecordings); } + UICommand::Undo => { + if let Some(store_context) = store_context { + let blueprint_id = store_context.blueprint.store_id().clone(); + self.command_sender + .send_system(SystemCommand::UndoBlueprint { blueprint_id }); + } + } + UICommand::Redo => { + if let Some(store_context) = store_context { + let blueprint_id = store_context.blueprint.store_id().clone(); + self.command_sender + .send_system(SystemCommand::RedoBlueprint { blueprint_id }); + } + } + #[cfg(not(target_arch = "wasm32"))] UICommand::Quit => { egui_ctx.send_viewport_cmd(egui::ViewportCommand::Close); @@ -1821,9 +1862,17 @@ impl eframe::App for App { { let store_context = store_hub.read_context(); + let blueprint_query = store_context.as_ref().map_or( + BlueprintUndoState::default_query(), + |store_context| { + self.state + .blueprint_query_for_viewer(store_context.blueprint) + }, + ); + let app_blueprint = AppBlueprint::new( store_context.as_ref(), - &self.state.blueprint_query_for_viewer(), + &blueprint_query, egui_ctx, self.panel_state_overrides_active .then_some(self.panel_state_overrides), diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 26f91108f3e9..c6a3c7ff1329 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -8,16 +8,16 @@ use re_smart_channel::ReceiveSet; use re_types::blueprint::components::PanelState; use re_ui::ContextExt as _; use re_viewer_context::{ - blueprint_timeline, AppOptions, ApplicationSelectionState, CommandSender, ComponentUiRegistry, - PlayState, RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, - StoreHub, SystemCommandSender as _, ViewStates, ViewerContext, + AppOptions, ApplicationSelectionState, CommandSender, ComponentUiRegistry, PlayState, + RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, StoreHub, + SystemCommandSender as _, ViewStates, ViewerContext, }; use re_viewport::Viewport; use re_viewport_blueprint::ui::add_space_view_or_container_modal_ui; use re_viewport_blueprint::ViewportBlueprint; -use crate::app_blueprint::AppBlueprint; use crate::ui::{recordings_panel_ui, settings_screen_ui}; +use crate::{app_blueprint::AppBlueprint, undo::BlueprintUndoState}; const WATERMARK: bool = false; // Nice for recording media material @@ -31,6 +31,9 @@ pub struct AppState { recording_configs: HashMap, pub blueprint_cfg: RecordingConfig, + /// Maps blueperint id to the current undo state for it. + pub blueprint_undo_state: HashMap, + selection_panel: re_selection_panel::SelectionPanel, time_panel: re_time_panel::TimePanel, blueprint_panel: re_time_panel::TimePanel, @@ -77,6 +80,7 @@ impl Default for AppState { Self { app_options: Default::default(), recording_configs: Default::default(), + blueprint_undo_state: Default::default(), blueprint_cfg: Default::default(), selection_panel: Default::default(), time_panel: Default::default(), @@ -148,11 +152,12 @@ impl AppState { ) { re_tracing::profile_function!(); - let blueprint_query = self.blueprint_query_for_viewer(); + let blueprint_query = self.blueprint_query_for_viewer(store_context.blueprint); let Self { app_options, recording_configs, + blueprint_undo_state, blueprint_cfg, selection_panel, time_panel, @@ -170,6 +175,11 @@ impl AppState { // check state early, before the UI has a chance to close these popups let is_any_popup_open = ui.memory(|m| m.any_popup_open()); + blueprint_undo_state + .entry(store_context.blueprint.store_id().clone()) + .or_default() + .update(ui.ctx(), store_context.blueprint); + // Some of the mutations APIs of `ViewportBlueprints` are recorded as `Viewport::TreeAction` // and must be applied by `Viewport` at the end of the frame. We use a temporary channel for // this, which gives us interior mutability (only a shared reference of `ViewportBlueprint` @@ -504,6 +514,9 @@ impl AppState { self.recording_configs .retain(|store_id, _| store_hub.store_bundle().contains(store_id)); + + self.blueprint_undo_state + .retain(|store_id, _| store_hub.store_bundle().contains(store_id)); } /// Returns the blueprint query that should be used for generating the current @@ -511,17 +524,21 @@ impl AppState { /// /// If `inspect_blueprint_timeline` is enabled, we use the time selection from the /// blueprint `time_ctrl`. Otherwise, we use a latest query from the blueprint timeline. - pub fn blueprint_query_for_viewer(&self) -> LatestAtQuery { + pub fn blueprint_query_for_viewer(&mut self, blueprint: &EntityDb) -> LatestAtQuery { if self.app_options.inspect_blueprint_timeline { let time_ctrl = self.blueprint_cfg.time_ctrl.read(); if time_ctrl.play_state() == PlayState::Following { // Special-case just to make sure we include stuff added in this frame - LatestAtQuery::latest(blueprint_timeline()) + LatestAtQuery::latest(re_viewer_context::blueprint_timeline()) } else { time_ctrl.current_query().clone() } } else { - LatestAtQuery::latest(blueprint_timeline()) + let undo_state = self + .blueprint_undo_state + .entry(blueprint.store_id().clone()) + .or_default(); + undo_state.blueprint_query() } } } diff --git a/crates/viewer/re_viewer/src/lib.rs b/crates/viewer/re_viewer/src/lib.rs index 60fef300f387..88d8d4065a7c 100644 --- a/crates/viewer/re_viewer/src/lib.rs +++ b/crates/viewer/re_viewer/src/lib.rs @@ -17,8 +17,11 @@ mod reflection; mod saving; mod screenshotter; mod ui; +mod undo; mod viewer_analytics; +pub use undo::BlueprintUndoState; + /// Auto-generated blueprint-related types. /// /// They all implement the [`re_types_core::Component`] trait. diff --git a/crates/viewer/re_viewer/src/ui/rerun_menu.rs b/crates/viewer/re_viewer/src/ui/rerun_menu.rs index e70f472cd09e..a4a2ff703347 100644 --- a/crates/viewer/re_viewer/src/ui/rerun_menu.rs +++ b/crates/viewer/re_viewer/src/ui/rerun_menu.rs @@ -42,6 +42,9 @@ impl App { ui.add_space(SPACING); + UICommand::Undo.menu_button_ui(ui, &self.command_sender); // TODO(emilk): only enabled if there is something to undo + UICommand::Redo.menu_button_ui(ui, &self.command_sender); // TODO(emilk): only enabled if there is something to redo + UICommand::ToggleCommandPalette.menu_button_ui(ui, &self.command_sender); ui.add_space(SPACING); diff --git a/crates/viewer/re_viewer/src/undo.rs b/crates/viewer/re_viewer/src/undo.rs new file mode 100644 index 000000000000..e30905fbd70c --- /dev/null +++ b/crates/viewer/re_viewer/src/undo.rs @@ -0,0 +1,120 @@ +use std::collections::BTreeSet; + +use re_chunk::{LatestAtQuery, TimeInt}; +use re_entity_db::EntityDb; +use re_log_types::ResolvedTimeRange; +use re_viewer_context::blueprint_timeline; + +/// We store the entire edit history of a blueprint in its store. +/// +/// When undoing, we move back time, and redoing move it forward. +/// When editing, we first drop all data after the current time. +#[derive(Clone, Debug, Default, serde::Deserialize, serde::Serialize)] +pub struct BlueprintUndoState { + /// The current blueprint time, used for latest-at. + /// + /// Everything _after_ this time is in "redo-space", + /// and will be dropped before new events are appended to the timeline. + /// + /// If `None`, use the max time of the blueprint timeline. + current_time: Option, + + /// Interesting times to undo/redo to. + /// + /// When the user drags a slider or similar, we get new events + /// recorded on each frame. The user presumably wants to undo the whole + /// slider drag, and not each increment of it. + /// + /// So we use a heuristic to estimate when such interactions start/stop, + /// and add them to this set. + inflection_points: BTreeSet, +} + +impl BlueprintUndoState { + /// Default latest-at query + #[inline] + pub fn default_query() -> LatestAtQuery { + LatestAtQuery::latest(blueprint_timeline()) + } + + pub fn blueprint_query(&self) -> LatestAtQuery { + if let Some(time) = self.current_time { + LatestAtQuery::new(blueprint_timeline(), time) + } else { + Self::default_query() + } + } + + pub fn undo(&mut self, blueprint_db: &EntityDb) { + let time = self + .current_time + .unwrap_or_else(|| max_blueprint_time(blueprint_db)); + + if let Some(previous) = self.inflection_points.range(..time).next_back().copied() { + self.current_time = Some(previous); + } else { + // nothing to undo to + } + } + + pub fn redo(&mut self, _blueprint_db: &EntityDb) { + if let Some(time) = self.current_time { + self.current_time = self.inflection_points.range(time.inc()..).next().copied(); + } else { + // If we have no time, we're at latest, and have nothing to redo + } + } + + pub fn clear_redo(&mut self, blueprint_db: &mut EntityDb) { + re_tracing::profile_function!(); + + if let Some(last_kept_event_time) = self.current_time.take() { + let first_dropped_event_time = + TimeInt::new_temporal(last_kept_event_time.as_i64().saturating_add(1)); + + // Drop everything before the current timeline time + blueprint_db.drop_time_range( + &blueprint_timeline(), + ResolvedTimeRange::new(first_dropped_event_time, re_chunk::TimeInt::MAX), + ); + } + } + + // Call each frame + pub fn update(&mut self, egui_ctx: &egui::Context, blueprint_db: &EntityDb) { + if is_interacting(egui_ctx) { + return; + } + + // Nothing is happening - remember this as a time to undo to. + let time = max_blueprint_time(blueprint_db); + let inserted = self.inflection_points.insert(time); + if inserted { + re_log::trace!("Inserted new inflection point at {time:?}"); + } + + // TODO(emilk): we should _also_ look for long streaks of changes (changes every frame) + // and disregard those, in case we miss something in `is_interacting`. + // Note that this on its own won't enough though - if you drag a slider, + // then you don't want an undo-point each time you pause the mouse - only on mouse-up! + } +} + +fn max_blueprint_time(blueprint_db: &EntityDb) -> TimeInt { + blueprint_db + .time_histogram(&blueprint_timeline()) + .and_then(|times| times.max_key()) + .map_or(TimeInt::ZERO, TimeInt::new_temporal) +} + +fn is_interacting(egui_ctx: &egui::Context) -> bool { + egui_ctx.input(|i| { + let is_scrolling = i.smooth_scroll_delta != egui::Vec2::ZERO; + let is_zooming = i.zoom_delta_2d() != egui::Vec2::splat(1.0); + i.pointer.any_down() + || i.any_touches() + || is_scrolling + || !i.keys_down.is_empty() + || is_zooming + }) +} diff --git a/crates/viewer/re_viewer_context/src/command_sender.rs b/crates/viewer/re_viewer_context/src/command_sender.rs index 0219a5d31230..a0abe954bcbe 100644 --- a/crates/viewer/re_viewer_context/src/command_sender.rs +++ b/crates/viewer/re_viewer_context/src/command_sender.rs @@ -49,6 +49,13 @@ pub enum SystemCommand { /// is both modified and changed in the same frame. UpdateBlueprint(StoreId, Vec), + UndoBlueprint { + blueprint_id: StoreId, + }, + RedoBlueprint { + blueprint_id: StoreId, + }, + /// Drop a specific entity from a store. /// /// Also drops all recursive children. diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 33a003dafb1d..a10e7aeb9a26 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -166,6 +166,8 @@ impl TestContext { | SystemCommand::ClearAndGenerateBlueprint | SystemCommand::ActivateRecording(_) | SystemCommand::CloseStore(_) + | SystemCommand::UndoBlueprint { .. } + | SystemCommand::RedoBlueprint { .. } | SystemCommand::CloseAllRecordings => handled = false, #[cfg(debug_assertions)] From a91a44525e688e5804bad3ef18dfaacddbd5806d Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 6 Oct 2024 17:22:57 +0200 Subject: [PATCH 02/20] Move `BlueprintUndoState` into `re_viewer_context` --- crates/viewer/re_viewer/src/app.rs | 8 ++++---- crates/viewer/re_viewer/src/app_state.rs | 12 +++++++----- crates/viewer/re_viewer/src/lib.rs | 3 --- crates/viewer/re_viewer_context/src/lib.rs | 2 ++ .../{re_viewer => re_viewer_context}/src/undo.rs | 3 ++- 5 files changed, 15 insertions(+), 13 deletions(-) rename crates/viewer/{re_viewer => re_viewer_context}/src/undo.rs (98%) diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index d76eacc9fddd..ce897d2f421b 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -10,16 +10,16 @@ use re_ui::{toasts, DesignTokens, UICommand, UICommandSender}; use re_viewer_context::{ command_channel, store_hub::{BlueprintPersistence, StoreHub, StoreHubStats}, - AppOptions, CommandReceiver, CommandSender, ComponentUiRegistry, PlayState, SpaceViewClass, - SpaceViewClassRegistry, SpaceViewClassRegistryError, StoreContext, SystemCommand, - SystemCommandSender, + AppOptions, BlueprintUndoState, CommandReceiver, CommandSender, ComponentUiRegistry, PlayState, + SpaceViewClass, SpaceViewClassRegistry, SpaceViewClassRegistryError, StoreContext, + SystemCommand, SystemCommandSender, }; +use crate::app_blueprint::PanelStateOverrides; use crate::{ app_blueprint::AppBlueprint, app_state::WelcomeScreenState, background_tasks::BackgroundTasks, AppState, }; -use crate::{app_blueprint::PanelStateOverrides, BlueprintUndoState}; // ---------------------------------------------------------------------------- diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index c6a3c7ff1329..1e63af84522b 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -8,16 +8,18 @@ use re_smart_channel::ReceiveSet; use re_types::blueprint::components::PanelState; use re_ui::ContextExt as _; use re_viewer_context::{ - AppOptions, ApplicationSelectionState, CommandSender, ComponentUiRegistry, PlayState, - RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, StoreHub, - SystemCommandSender as _, ViewStates, ViewerContext, + AppOptions, ApplicationSelectionState, BlueprintUndoState, CommandSender, ComponentUiRegistry, + PlayState, RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, + StoreHub, SystemCommandSender as _, ViewStates, ViewerContext, }; use re_viewport::Viewport; use re_viewport_blueprint::ui::add_space_view_or_container_modal_ui; use re_viewport_blueprint::ViewportBlueprint; -use crate::ui::{recordings_panel_ui, settings_screen_ui}; -use crate::{app_blueprint::AppBlueprint, undo::BlueprintUndoState}; +use crate::{ + app_blueprint::AppBlueprint, + ui::{recordings_panel_ui, settings_screen_ui}, +}; const WATERMARK: bool = false; // Nice for recording media material diff --git a/crates/viewer/re_viewer/src/lib.rs b/crates/viewer/re_viewer/src/lib.rs index 88d8d4065a7c..60fef300f387 100644 --- a/crates/viewer/re_viewer/src/lib.rs +++ b/crates/viewer/re_viewer/src/lib.rs @@ -17,11 +17,8 @@ mod reflection; mod saving; mod screenshotter; mod ui; -mod undo; mod viewer_analytics; -pub use undo::BlueprintUndoState; - /// Auto-generated blueprint-related types. /// /// They all implement the [`re_types_core::Component`] trait. diff --git a/crates/viewer/re_viewer_context/src/lib.rs b/crates/viewer/re_viewer_context/src/lib.rs index 0cd14c563bfb..59aa0a339f01 100644 --- a/crates/viewer/re_viewer_context/src/lib.rs +++ b/crates/viewer/re_viewer_context/src/lib.rs @@ -28,6 +28,7 @@ pub mod test_context; //TODO(ab): this should be behind #[cfg(test)], but then ` mod time_control; mod time_drag_value; mod typed_entity_collections; +mod undo; mod utils; mod viewer_context; @@ -83,6 +84,7 @@ pub use time_drag_value::TimeDragValue; pub use typed_entity_collections::{ ApplicableEntities, IndicatedEntities, PerVisualizer, VisualizableEntities, }; +pub use undo::BlueprintUndoState; pub use utils::{auto_color_egui, auto_color_for_entity_path, level_to_rich_text}; pub use viewer_context::{RecordingConfig, ViewerContext}; diff --git a/crates/viewer/re_viewer/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs similarity index 98% rename from crates/viewer/re_viewer/src/undo.rs rename to crates/viewer/re_viewer_context/src/undo.rs index e30905fbd70c..d9bad8edf2c1 100644 --- a/crates/viewer/re_viewer/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -3,7 +3,8 @@ use std::collections::BTreeSet; use re_chunk::{LatestAtQuery, TimeInt}; use re_entity_db::EntityDb; use re_log_types::ResolvedTimeRange; -use re_viewer_context::blueprint_timeline; + +use crate::blueprint_timeline; /// We store the entire edit history of a blueprint in its store. /// From 686a20041fe7107d40c7db5604d11318a99e1302 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 6 Oct 2024 17:32:12 +0200 Subject: [PATCH 03/20] When running GC on the blueprint, protect the latest 100 undo points --- crates/store/re_entity_db/src/entity_db.rs | 17 ++-------- crates/store/re_entity_db/src/lib.rs | 2 +- crates/viewer/re_viewer/src/app.rs | 4 +-- .../viewer/re_viewer_context/src/store_hub.rs | 32 ++++++++++++++----- crates/viewer/re_viewer_context/src/undo.rs | 19 +++++++++++ 5 files changed, 48 insertions(+), 26 deletions(-) diff --git a/crates/store/re_entity_db/src/entity_db.rs b/crates/store/re_entity_db/src/entity_db.rs index 2cc64e17b2f3..ee9c3d09a46c 100644 --- a/crates/store/re_entity_db/src/entity_db.rs +++ b/crates/store/re_entity_db/src/entity_db.rs @@ -22,7 +22,7 @@ use crate::{Error, TimesPerTimeline}; // ---------------------------------------------------------------------------- /// See [`GarbageCollectionOptions::time_budget`]. -const DEFAULT_GC_TIME_BUDGET: std::time::Duration = std::time::Duration::from_micros(3500); // empirical +pub const DEFAULT_GC_TIME_BUDGET: std::time::Duration = std::time::Duration::from_micros(3500); // empirical // ---------------------------------------------------------------------------- @@ -408,19 +408,6 @@ impl EntityDb { self.set_store_info = Some(store_info); } - pub fn gc_everything_but_the_latest_row_on_non_default_timelines( - &mut self, - ) -> Vec { - re_tracing::profile_function!(); - - self.gc(&GarbageCollectionOptions { - target: GarbageCollectionTarget::Everything, - protect_latest: 1, - time_budget: DEFAULT_GC_TIME_BUDGET, - protected_time_ranges: Default::default(), // TODO(#3135): Use this for undo buffer - }) - } - /// Free up some RAM by forgetting the older parts of all timelines. pub fn purge_fraction_of_ram(&mut self, fraction_to_purge: f32) -> Vec { re_tracing::profile_function!(); @@ -454,7 +441,7 @@ impl EntityDb { store_events } - pub(crate) fn gc(&mut self, gc_options: &GarbageCollectionOptions) -> Vec { + pub fn gc(&mut self, gc_options: &GarbageCollectionOptions) -> Vec { re_tracing::profile_function!(); let mut engine = self.storage_engine.write(); diff --git a/crates/store/re_entity_db/src/lib.rs b/crates/store/re_entity_db/src/lib.rs index 0addb29084ef..b2f85969e409 100644 --- a/crates/store/re_entity_db/src/lib.rs +++ b/crates/store/re_entity_db/src/lib.rs @@ -13,7 +13,7 @@ mod times_per_timeline; mod versioned_instance_path; pub use self::{ - entity_db::EntityDb, + entity_db::{EntityDb, DEFAULT_GC_TIME_BUDGET}, entity_tree::EntityTree, instance_path::{InstancePath, InstancePathHash}, store_bundle::{StoreBundle, StoreLoadError}, diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index ce897d2f421b..8f550d297ffd 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -1707,7 +1707,7 @@ impl eframe::App for App { // TODO(#2579): implement web-storage for blueprints as well if let Some(hub) = &mut self.store_hub { if self.state.app_options.blueprint_gc { - hub.gc_blueprints(); + hub.gc_blueprints(&self.state.blueprint_undo_state); } if let Err(err) = hub.save_app_blueprints() { @@ -1835,7 +1835,7 @@ impl eframe::App for App { self.receive_messages(&mut store_hub, egui_ctx); if self.app_options().blueprint_gc { - store_hub.gc_blueprints(); + store_hub.gc_blueprints(&self.state.blueprint_undo_state); } store_hub.purge_empty(); diff --git a/crates/viewer/re_viewer_context/src/store_hub.rs b/crates/viewer/re_viewer_context/src/store_hub.rs index 192990f46ae2..e7440e3d80bc 100644 --- a/crates/viewer/re_viewer_context/src/store_hub.rs +++ b/crates/viewer/re_viewer_context/src/store_hub.rs @@ -3,12 +3,15 @@ use ahash::{HashMap, HashMapExt, HashSet}; use anyhow::Context as _; use itertools::Itertools as _; -use re_chunk_store::{ChunkStoreConfig, ChunkStoreGeneration, ChunkStoreStats}; +use re_chunk_store::{ + ChunkStoreConfig, ChunkStoreGeneration, ChunkStoreStats, GarbageCollectionOptions, + GarbageCollectionTarget, +}; use re_entity_db::{EntityDb, StoreBundle}; -use re_log_types::{ApplicationId, StoreId, StoreKind}; +use re_log_types::{ApplicationId, ResolvedTimeRange, StoreId, StoreKind}; use re_query::CachesStats; -use crate::{Caches, StoreContext}; +use crate::{BlueprintUndoState, Caches, StoreContext}; /// Interface for accessing all blueprints and recordings /// @@ -683,7 +686,7 @@ impl StoreHub { }); } - pub fn gc_blueprints(&mut self) { + pub fn gc_blueprints(&mut self, undo_state: &HashMap) { re_tracing::profile_function!(); for blueprint_id in self @@ -696,10 +699,23 @@ impl StoreHub { continue; // no change since last gc } - // TODO(jleibs): Decide a better tuning for this. Would like to save a - // reasonable amount of history, or incremental snapshots. - let store_events = - blueprint.gc_everything_but_the_latest_row_on_non_default_timelines(); + let mut protected_time_ranges = ahash::HashMap::default(); + if let Some(undo) = undo_state.get(blueprint_id) { + if let Some(time) = undo.oldest_undo_point() { + // Save everything that we could want to undo to: + protected_time_ranges.insert( + crate::blueprint_timeline(), + ResolvedTimeRange::new(time, re_chunk::TimeInt::MAX), + ); + } + } + + let store_events = blueprint.gc(&GarbageCollectionOptions { + target: GarbageCollectionTarget::Everything, + protect_latest: 0, + time_budget: re_entity_db::DEFAULT_GC_TIME_BUDGET, + protected_time_ranges, + }); if let Some(caches) = self.caches_per_recording.get_mut(blueprint_id) { caches.on_store_events(&store_events); } diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index d9bad8edf2c1..836e52976725 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -6,6 +6,11 @@ use re_log_types::ResolvedTimeRange; use crate::blueprint_timeline; +/// Max number of undo points. +/// +/// TODO(emilk): decide based on how much memory the blueprint uses instead. +const MAX_UNDOS: usize = 100; + /// We store the entire edit history of a blueprint in its store. /// /// When undoing, we move back time, and redoing move it forward. @@ -38,6 +43,11 @@ impl BlueprintUndoState { LatestAtQuery::latest(blueprint_timeline()) } + /// How far back in time can we undo? + pub fn oldest_undo_point(&self) -> Option { + self.inflection_points.first().copied() + } + pub fn blueprint_query(&self) -> LatestAtQuery { if let Some(time) = self.current_time { LatestAtQuery::new(blueprint_timeline(), time) @@ -98,6 +108,15 @@ impl BlueprintUndoState { // and disregard those, in case we miss something in `is_interacting`. // Note that this on its own won't enough though - if you drag a slider, // then you don't want an undo-point each time you pause the mouse - only on mouse-up! + + // Don't store too many undo-points: + while let Some(first) = self.inflection_points.first().copied() { + if MAX_UNDOS < self.inflection_points.len() { + self.inflection_points.remove(&first); + } else { + break; + } + } } } From ea753ebe508e2e207af57cfa69bcd965e0645e43 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 6 Oct 2024 17:16:19 +0200 Subject: [PATCH 04/20] Do not remove entities from the blueprint datastore --- .../re_viewport_blueprint/src/container.rs | 9 --------- .../re_viewport_blueprint/src/space_view.rs | 7 ------- .../src/viewport_blueprint.rs | 16 ++-------------- 3 files changed, 2 insertions(+), 30 deletions(-) diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 70f9d1059e18..23b83a61e5a3 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -348,15 +348,6 @@ impl ContainerBlueprint { } } - /// Clears the blueprint component for this container. - // TODO(jleibs): Should this be a recursive clear? - pub fn clear(&self, ctx: &ViewerContext<'_>) { - ctx.command_sender.send_system(SystemCommand::DropEntity( - ctx.store_context.blueprint.store_id().clone(), - self.entity_path(), - )); - } - pub fn to_tile(&self) -> egui_tiles::Tile { let children = self .contents diff --git a/crates/viewer/re_viewport_blueprint/src/space_view.rs b/crates/viewer/re_viewport_blueprint/src/space_view.rs index bc04480d9fe5..13d00c02cfa5 100644 --- a/crates/viewer/re_viewport_blueprint/src/space_view.rs +++ b/crates/viewer/re_viewport_blueprint/src/space_view.rs @@ -308,13 +308,6 @@ impl SpaceViewBlueprint { } } - pub fn clear(&self, ctx: &ViewerContext<'_>) { - ctx.command_sender.send_system(SystemCommand::DropEntity( - ctx.store_context.blueprint.store_id().clone(), - self.entity_path(), - )); - } - #[inline] pub fn set_display_name(&self, ctx: &ViewerContext<'_>, name: Option) { if name != self.display_name { diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index acd66e2f5b94..a6bf13014fd0 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -95,6 +95,8 @@ impl ViewportBlueprint { let root_container: Option = root_container.map(|id| id.0.into()); re_log::trace_once!("Loaded root_container: {root_container:?}"); + // This visits all space views that ever has been, which is likely more than exist now. + // TODO(emilk): optimize this by starting at the root and only visit reachable space viewa. let all_space_view_ids: Vec = blueprint_db .tree() .children @@ -212,11 +214,6 @@ impl ViewportBlueprint { pub fn remove_space_view(&self, space_view_id: &SpaceViewId, ctx: &ViewerContext<'_>) { self.mark_user_interaction(ctx); - // 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) { self.set_maximized(None, ctx); @@ -867,15 +864,6 @@ impl ViewportBlueprint { } } - // Clear any existing container blueprints that aren't referenced - // by any tiles. - for (container_id, container) in &self.containers { - let tile_id = blueprint_id_to_tile_id(container_id); - if tree.tiles.get(tile_id).is_none() { - container.clear(ctx); - } - } - // Now save any contents that are a container back to the blueprint for (tile_id, contents) in &contents_from_tile_id { if let Contents::Container(container_id) = contents { From 8de94c282e95b1fdeab40fe366c9c71fc4afa5d2 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 6 Oct 2024 18:43:43 +0200 Subject: [PATCH 05/20] THIS COMMIT MAKES SENSE BUT BREAKS THE VIEWPORT --- crates/viewer/re_viewport/src/viewport.rs | 11 +++- .../src/viewport_blueprint.rs | 55 ++++++++----------- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/crates/viewer/re_viewport/src/viewport.rs b/crates/viewer/re_viewport/src/viewport.rs index 3a52cf3111c7..73feb2997c40 100644 --- a/crates/viewer/re_viewport/src/viewport.rs +++ b/crates/viewer/re_viewport/src/viewport.rs @@ -74,10 +74,15 @@ impl<'a> Viewport<'a> { // If the blueprint tree is empty/missing we need to auto-layout. let tree = if blueprint.tree.is_empty() { edited = true; - super::auto_layout::tree_from_space_views( + let auto_tree = super::auto_layout::tree_from_space_views( space_view_class_registry, &blueprint.space_views, - ) + ); + re_log::trace!( + "New auto-tree: {auto_tree:#?} based on {} space_views", + blueprint.space_views.len() + ); + auto_tree } else { blueprint.tree.clone() }; @@ -425,6 +430,8 @@ impl<'a> Viewport<'a> { if self.tree_edited { // TODO(#4687): Be extra careful here. If we mark edited inappropriately we can create an infinite edit loop. + re_log::trace!("Saving edited tree: {:#?}", self.tree); + // Simplify before we save the tree. Normally additional simplification will // happen on the next render loop, but that's too late -- unsimplified // changes will be baked into the tree. diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index a6bf13014fd0..6a1d228ace7b 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -4,12 +4,12 @@ use std::sync::atomic::{AtomicBool, Ordering}; use ahash::HashMap; use egui_tiles::{SimplificationOptions, TileId}; use nohash_hasher::IntSet; -use re_types::{Archetype as _, SpaceViewClassIdentifier}; use smallvec::SmallVec; use re_chunk_store::LatestAtQuery; use re_entity_db::EntityPath; use re_types::blueprint::components::ViewerRecommendationHash; +use re_types::{Archetype as _, SpaceViewClassIdentifier}; use re_types_blueprint::blueprint::archetypes as blueprint_archetypes; use re_types_blueprint::blueprint::components::{ AutoLayout, AutoSpaceViews, IncludedSpaceView, RootContainer, SpaceViewMaximized, @@ -95,19 +95,26 @@ impl ViewportBlueprint { let root_container: Option = root_container.map(|id| id.0.into()); re_log::trace_once!("Loaded root_container: {root_container:?}"); - // This visits all space views that ever has been, which is likely more than exist now. - // TODO(emilk): optimize this by starting at the root and only visit reachable space viewa. - let all_space_view_ids: Vec = blueprint_db - .tree() - .children - .get(SpaceViewId::registry_part()) - .map(|tree| { - tree.children - .values() - .map(|subtree| SpaceViewId::from_entity_path(&subtree.path)) - .collect() - }) - .unwrap_or_default(); + let mut containers: BTreeMap = Default::default(); + let mut all_space_view_ids: Vec = Default::default(); + + if let Some(root_container) = root_container { + re_tracing::profile_scope!("visit_all_containers"); + let mut container_ids_to_visit: Vec = vec![root_container]; + while let Some(id) = container_ids_to_visit.pop() { + if let Some(container) = ContainerBlueprint::try_from_db(blueprint_db, query, id) { + for &content in &container.contents { + match content { + Contents::Container(id) => container_ids_to_visit.push(id), + Contents::SpaceView(id) => { + all_space_view_ids.push(id); + } + } + } + containers.insert(id, container); + } + } + } let space_views: BTreeMap = all_space_view_ids .into_iter() @@ -117,24 +124,6 @@ impl ViewportBlueprint { .map(|sv| (sv.id, sv)) .collect(); - let all_container_ids: Vec = blueprint_db - .tree() - .children - .get(ContainerId::registry_part()) - .map(|tree| { - tree.children - .values() - .map(|subtree| ContainerId::from_entity_path(&subtree.path)) - .collect() - }) - .unwrap_or_default(); - - let containers: BTreeMap = all_container_ids - .into_iter() - .filter_map(|id| ContainerBlueprint::try_from_db(blueprint_db, query, id)) - .map(|c| (c.id, c)) - .collect(); - // Auto layouting and auto space view are only enabled if no blueprint has been provided by the user. // Only enable auto-space-views if this is the app-default blueprint let is_app_default_blueprint = blueprint_db @@ -890,8 +879,10 @@ impl ViewportBlueprint { .and_then(|contents| contents.as_container_id()) .map(|container_id| RootContainer((container_id).into())) { + re_log::trace!("Saving with a root container"); ctx.save_blueprint_component(&VIEWPORT_PATH.into(), &root_container); } else { + re_log::trace!("Saving empty viewport"); ctx.save_empty_blueprint_component::(&VIEWPORT_PATH.into()); } } From 0e222d6abba0d28aa3510d98d3ead2173aacf400 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 8 Nov 2024 15:27:57 +0100 Subject: [PATCH 06/20] Better logging of viewport-blueprint edits --- crates/viewer/re_viewport/src/viewport.rs | 2 +- .../re_viewport_blueprint/src/container.rs | 20 ++++++++++++++++++- .../src/viewport_blueprint.rs | 17 ++++++++-------- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/crates/viewer/re_viewport/src/viewport.rs b/crates/viewer/re_viewport/src/viewport.rs index 73feb2997c40..4a9891ee778f 100644 --- a/crates/viewer/re_viewport/src/viewport.rs +++ b/crates/viewer/re_viewport/src/viewport.rs @@ -79,7 +79,7 @@ impl<'a> Viewport<'a> { &blueprint.space_views, ); re_log::trace!( - "New auto-tree: {auto_tree:#?} based on {} space_views", + "Auto-generated new tree based on {} space_views: {auto_tree:#?}", blueprint.space_views.len() ); auto_tree diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 23b83a61e5a3..e7c451588eb1 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -22,7 +22,7 @@ use re_viewer_context::{ /// /// The main reason this exists is to handle type conversions that aren't yet /// well handled by the code-generated archetypes. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ContainerBlueprint { pub id: ContainerId, pub container_kind: egui_tiles::ContainerKind, @@ -139,6 +139,24 @@ impl ContainerBlueprint { self.id.as_entity_path() } + pub fn add_child(&mut self, content: Contents) { + self.contents.push(content); + match self.container_kind { + egui_tiles::ContainerKind::Tabs => { + self.active_tab = self.active_tab.or(Some(content)); + } + egui_tiles::ContainerKind::Horizontal => { + self.col_shares.push(1.0); + } + egui_tiles::ContainerKind::Vertical => { + self.row_shares.push(1.0); + } + egui_tiles::ContainerKind::Grid => { + // dunno + } + } + } + /// Persist the entire [`ContainerBlueprint`] to the blueprint store. /// /// This only needs to be called if the [`ContainerBlueprint`] was created with [`Self::from_egui_tiles_container`]. diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index 6a1d228ace7b..40f8e2036c7e 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -103,6 +103,7 @@ impl ViewportBlueprint { let mut container_ids_to_visit: Vec = vec![root_container]; while let Some(id) = container_ids_to_visit.pop() { if let Some(container) = ContainerBlueprint::try_from_db(blueprint_db, query, id) { + re_log::trace_once!("Container {id} contents: {:?}", container.contents); for &content in &container.contents { match content { Contents::Container(id) => container_ids_to_visit.push(id), @@ -112,6 +113,8 @@ impl ViewportBlueprint { } } containers.insert(id, container); + } else { + re_log::warn_once!("Failed to load container {id}"); } } } @@ -419,14 +422,12 @@ impl ViewportBlueprint { new_ids.push(space_view_id); } - if !new_ids.is_empty() { - for id in &new_ids { - self.send_tree_action(TreeAction::AddSpaceView( - *id, - parent_container, - position_in_parent, - )); - } + for id in &new_ids { + self.send_tree_action(TreeAction::AddSpaceView( + *id, + parent_container, + position_in_parent, + )); } new_ids From 3b10beb743a419f53959c5fcb4e8b0c3470f9237 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 8 Nov 2024 16:46:35 +0100 Subject: [PATCH 07/20] Get it to work --- crates/viewer/re_viewport/src/viewport.rs | 4 +- .../re_viewport_blueprint/src/tree_actions.rs | 4 +- .../src/viewport_blueprint.rs | 40 ++++++++++++++++--- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/crates/viewer/re_viewport/src/viewport.rs b/crates/viewer/re_viewport/src/viewport.rs index 4a9891ee778f..78ddc1b1b4c8 100644 --- a/crates/viewer/re_viewport/src/viewport.rs +++ b/crates/viewer/re_viewport/src/viewport.rs @@ -242,7 +242,9 @@ impl<'a> Viewport<'a> { re_log::trace!("Processing tree action: {tree_action:?}"); match tree_action { TreeAction::AddSpaceView(space_view_id, parent_container, position_in_parent) => { - if self.blueprint.auto_layout() { + // if self.blueprint.auto_layout() + if false { + // TODO: why was this code here? // Re-run the auto-layout next frame: re_log::trace!( "Added a space view with no user edits yet - will re-run auto-layout" diff --git a/crates/viewer/re_viewport_blueprint/src/tree_actions.rs b/crates/viewer/re_viewport_blueprint/src/tree_actions.rs index 6e9ab96f4ed7..f7dee7cf12e2 100644 --- a/crates/viewer/re_viewport_blueprint/src/tree_actions.rs +++ b/crates/viewer/re_viewport_blueprint/src/tree_actions.rs @@ -5,10 +5,10 @@ use re_viewer_context::{ContainerId, Contents, SpaceViewId}; #[derive(Clone, Debug)] pub enum TreeAction { /// Add a new space view to the provided container (or the root if `None`). - AddSpaceView(SpaceViewId, Option, Option), + AddSpaceView(SpaceViewId, Option, Option), // TODO: name fields /// Add a new container of the provided kind to the provided container (or the root if `None`). - AddContainer(egui_tiles::ContainerKind, Option), + AddContainer(egui_tiles::ContainerKind, Option), // TODO: name fields /// Change the kind of a container. SetContainerKind(ContainerId, egui_tiles::ContainerKind), diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index 40f8e2036c7e..bb4e23363a2e 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -410,6 +410,8 @@ impl ViewportBlueprint { parent_container: Option, position_in_parent: Option, ) -> Vec { + let parent_container_id = parent_container.unwrap_or(self.root_container); + let mut new_ids: Vec<_> = vec![]; for space_view in space_views { @@ -422,12 +424,38 @@ impl ViewportBlueprint { new_ids.push(space_view_id); } - for id in &new_ids { - self.send_tree_action(TreeAction::AddSpaceView( - *id, - parent_container, - position_in_parent, - )); + if !new_ids.is_empty() { + let mut container = self + .containers + .get(&parent_container_id) + .cloned() + .unwrap_or_else(|| { + self.send_tree_action(TreeAction::AddContainer( + egui_tiles::ContainerKind::Grid, + None, + )); + ContainerBlueprint { + id: parent_container_id, + ..Default::default() + } + }); + for &space_view_id in &new_ids { + container.add_child(Contents::SpaceView(space_view_id)); + } + re_log::trace!( + "Added {} space-views to container {parent_container_id}; now contains: {:?}", + new_ids.len(), + container.contents, + ); + container.save_to_blueprint_store(ctx); + + for id in &new_ids { + self.send_tree_action(TreeAction::AddSpaceView( + *id, + Some(parent_container_id), + position_in_parent, + )); + } } new_ids From 7f23775526bcbd1e1d1fac7bdcd2460fc6d46d02 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 11 Nov 2024 09:16:29 +0100 Subject: [PATCH 08/20] Better logging --- crates/viewer/re_viewer_context/src/store_hub.rs | 7 +++++-- crates/viewer/re_viewer_context/src/undo.rs | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/store_hub.rs b/crates/viewer/re_viewer_context/src/store_hub.rs index e7440e3d80bc..6adf3958acc0 100644 --- a/crates/viewer/re_viewer_context/src/store_hub.rs +++ b/crates/viewer/re_viewer_context/src/store_hub.rs @@ -716,8 +716,11 @@ impl StoreHub { time_budget: re_entity_db::DEFAULT_GC_TIME_BUDGET, protected_time_ranges, }); - if let Some(caches) = self.caches_per_recording.get_mut(blueprint_id) { - caches.on_store_events(&store_events); + if !store_events.is_empty() { + re_log::debug!("Garbage-collected blueprint store"); + if let Some(caches) = self.caches_per_recording.get_mut(blueprint_id) { + caches.on_store_events(&store_events); + } } self.blueprint_last_gc diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index 836e52976725..bac88665bb5f 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -65,6 +65,7 @@ impl BlueprintUndoState { self.current_time = Some(previous); } else { // nothing to undo to + re_log::debug!("Nothing to undo"); } } @@ -73,6 +74,7 @@ impl BlueprintUndoState { self.current_time = self.inflection_points.range(time.inc()..).next().copied(); } else { // If we have no time, we're at latest, and have nothing to redo + re_log::debug!("Nothing to redo"); } } From f15970362b77f8a3cefcc00a5af36a6b8fd11437 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 28 Nov 2024 17:01:56 +0100 Subject: [PATCH 09/20] Undo changes that's just gonna result in merge conflicts --- crates/viewer/re_viewport/src/viewport.rs | 15 +++------------ .../re_viewport_blueprint/src/tree_actions.rs | 4 ++-- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/crates/viewer/re_viewport/src/viewport.rs b/crates/viewer/re_viewport/src/viewport.rs index 78ddc1b1b4c8..3a52cf3111c7 100644 --- a/crates/viewer/re_viewport/src/viewport.rs +++ b/crates/viewer/re_viewport/src/viewport.rs @@ -74,15 +74,10 @@ impl<'a> Viewport<'a> { // If the blueprint tree is empty/missing we need to auto-layout. let tree = if blueprint.tree.is_empty() { edited = true; - let auto_tree = super::auto_layout::tree_from_space_views( + super::auto_layout::tree_from_space_views( space_view_class_registry, &blueprint.space_views, - ); - re_log::trace!( - "Auto-generated new tree based on {} space_views: {auto_tree:#?}", - blueprint.space_views.len() - ); - auto_tree + ) } else { blueprint.tree.clone() }; @@ -242,9 +237,7 @@ impl<'a> Viewport<'a> { re_log::trace!("Processing tree action: {tree_action:?}"); match tree_action { TreeAction::AddSpaceView(space_view_id, parent_container, position_in_parent) => { - // if self.blueprint.auto_layout() - if false { - // TODO: why was this code here? + 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" @@ -432,8 +425,6 @@ impl<'a> Viewport<'a> { if self.tree_edited { // TODO(#4687): Be extra careful here. If we mark edited inappropriately we can create an infinite edit loop. - re_log::trace!("Saving edited tree: {:#?}", self.tree); - // Simplify before we save the tree. Normally additional simplification will // happen on the next render loop, but that's too late -- unsimplified // changes will be baked into the tree. diff --git a/crates/viewer/re_viewport_blueprint/src/tree_actions.rs b/crates/viewer/re_viewport_blueprint/src/tree_actions.rs index f7dee7cf12e2..6e9ab96f4ed7 100644 --- a/crates/viewer/re_viewport_blueprint/src/tree_actions.rs +++ b/crates/viewer/re_viewport_blueprint/src/tree_actions.rs @@ -5,10 +5,10 @@ use re_viewer_context::{ContainerId, Contents, SpaceViewId}; #[derive(Clone, Debug)] pub enum TreeAction { /// Add a new space view to the provided container (or the root if `None`). - AddSpaceView(SpaceViewId, Option, Option), // TODO: name fields + AddSpaceView(SpaceViewId, Option, Option), /// Add a new container of the provided kind to the provided container (or the root if `None`). - AddContainer(egui_tiles::ContainerKind, Option), // TODO: name fields + AddContainer(egui_tiles::ContainerKind, Option), /// Change the kind of a container. SetContainerKind(ContainerId, egui_tiles::ContainerKind), From 9e8a6a37e50989f4e6f2a051f8a4ae0355fd807a Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 29 Nov 2024 10:03:53 +0100 Subject: [PATCH 10/20] Use a recursive clear when removing a container or space view --- .../viewer/re_viewer_context/src/store_hub.rs | 3 ++- crates/viewer/re_viewer_context/src/undo.rs | 3 ++- .../re_viewport_blueprint/src/container.rs | 23 +++++++++++++++---- .../re_viewport_blueprint/src/space_view.rs | 23 +++++++++++++++---- .../src/viewport_blueprint.rs | 5 ++-- 5 files changed, 42 insertions(+), 15 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/store_hub.rs b/crates/viewer/re_viewer_context/src/store_hub.rs index 6adf3958acc0..977ac4f3e167 100644 --- a/crates/viewer/re_viewer_context/src/store_hub.rs +++ b/crates/viewer/re_viewer_context/src/store_hub.rs @@ -711,8 +711,9 @@ impl StoreHub { } let store_events = blueprint.gc(&GarbageCollectionOptions { + // TODO(#8249): configure blueprint GC to remove an entity if all that remains of it is a recursive clear target: GarbageCollectionTarget::Everything, - protect_latest: 0, + protect_latest: 1, // keep the latest instance of everything, or we will forget things that haven't changed in a while time_budget: re_entity_db::DEFAULT_GC_TIME_BUDGET, protected_time_ranges, }); diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index bac88665bb5f..b7e0ca229e81 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -78,6 +78,7 @@ impl BlueprintUndoState { } } + /// Clears the "redo buffer". pub fn clear_redo(&mut self, blueprint_db: &mut EntityDb) { re_tracing::profile_function!(); @@ -85,7 +86,7 @@ impl BlueprintUndoState { let first_dropped_event_time = TimeInt::new_temporal(last_kept_event_time.as_i64().saturating_add(1)); - // Drop everything before the current timeline time + // Drop everything after the current timeline time blueprint_db.drop_time_range( &blueprint_timeline(), ResolvedTimeRange::new(first_dropped_event_time, re_chunk::TimeInt::MAX), diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 7c0e61e3169e..678148ba46e9 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -375,11 +375,24 @@ impl ContainerBlueprint { /// Clears the blueprint component for this container. pub fn clear(&self, ctx: &ViewerContext<'_>) { - // TODO: ecursive clear - ctx.command_sender.send_system(SystemCommand::DropEntity( - ctx.store_context.blueprint.store_id().clone(), - self.entity_path(), - )); + // We can't delete the entity, because we need to support undo. + // TODO(#8249): configure blueprint GC to remove this entity if all that remains is the recursive clear. + let timepoint = ctx.store_context.blueprint_timepoint_for_writes(); + + let chunk = Chunk::builder(self.entity_path()) + .with_archetype( + RowId::new(), + timepoint.clone(), + &re_types::archetypes::Clear::recursive(), + ) + .build() + .unwrap(); + + ctx.command_sender + .send_system(SystemCommand::UpdateBlueprint( + ctx.store_context.blueprint.store_id().clone(), + vec![chunk], + )); } pub fn to_tile(&self) -> egui_tiles::Tile { diff --git a/crates/viewer/re_viewport_blueprint/src/space_view.rs b/crates/viewer/re_viewport_blueprint/src/space_view.rs index cb05dd6cdfef..70ca57f8b98a 100644 --- a/crates/viewer/re_viewport_blueprint/src/space_view.rs +++ b/crates/viewer/re_viewport_blueprint/src/space_view.rs @@ -310,11 +310,24 @@ impl SpaceViewBlueprint { } pub fn clear(&self, ctx: &ViewerContext<'_>) { - // TODO: log a recursive clear - ctx.command_sender.send_system(SystemCommand::DropEntity( - ctx.store_context.blueprint.store_id().clone(), - self.entity_path(), - )); + // We can't delete the entity, because we need to support undo. + // TODO(#8249): configure blueprint GC to remove this entity if all that remains is the recursive clear. + let timepoint = ctx.store_context.blueprint_timepoint_for_writes(); + + let chunk = Chunk::builder(self.entity_path()) + .with_archetype( + RowId::new(), + timepoint.clone(), + &re_types::archetypes::Clear::recursive(), + ) + .build() + .unwrap(); + + ctx.command_sender + .send_system(SystemCommand::UpdateBlueprint( + ctx.store_context.blueprint.store_id().clone(), + vec![chunk], + )); } #[inline] diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index ae9c95883353..239865b24404 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -841,9 +841,8 @@ impl ViewportBlueprint { } } - // TODO: log a recursive clear here - // Clear any existing container blueprints that aren't referenced - // by any tiles. + // Clear any existing container blueprints that aren't referenced by any tiles, + // allowing the GC to remove the previous (non-clear) data from the store (saving RAM). for (container_id, container) in &self.containers { let tile_id = blueprint_id_to_tile_id(container_id); if self.tree.tiles.get(tile_id).is_none() { From 4676872127403878a0aaf7327023cd474dd9d74a Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 29 Nov 2024 10:10:08 +0100 Subject: [PATCH 11/20] Remove unwraps --- crates/viewer/re_viewport_blueprint/src/container.rs | 2 +- crates/viewer/re_viewport_blueprint/src/space_view.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 678148ba46e9..52e583f3b1a6 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -386,7 +386,7 @@ impl ContainerBlueprint { &re_types::archetypes::Clear::recursive(), ) .build() - .unwrap(); + .expect("Failed to serialize Clear component !?"); ctx.command_sender .send_system(SystemCommand::UpdateBlueprint( diff --git a/crates/viewer/re_viewport_blueprint/src/space_view.rs b/crates/viewer/re_viewport_blueprint/src/space_view.rs index 70ca57f8b98a..1d2cad6ec5cf 100644 --- a/crates/viewer/re_viewport_blueprint/src/space_view.rs +++ b/crates/viewer/re_viewport_blueprint/src/space_view.rs @@ -321,7 +321,7 @@ impl SpaceViewBlueprint { &re_types::archetypes::Clear::recursive(), ) .build() - .unwrap(); + .expect("Failed to serialize Clear component !?"); ctx.command_sender .send_system(SystemCommand::UpdateBlueprint( From d69b1a28b7047c6fd56d63497bf6053928f1f8e4 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 29 Nov 2024 23:30:52 +0100 Subject: [PATCH 12/20] Sync undo-state and blueprint time panel --- crates/viewer/re_viewer/src/app.rs | 3 +- crates/viewer/re_viewer/src/app_state.rs | 42 ++++++++++++++++++--- crates/viewer/re_viewer_context/src/undo.rs | 16 +++++++- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index fe478cdc6034..6a2a255bacb1 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -562,12 +562,11 @@ impl App { .undo(blueprint_db); } SystemCommand::RedoBlueprint { blueprint_id } => { - let blueprint_db = store_hub.entity_db_mut(&blueprint_id); self.state .blueprint_undo_state .entry(blueprint_id) .or_default() - .redo(blueprint_db); + .redo(); } SystemCommand::DropEntity(blueprint_id, entity_path) => { diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 8331c9bd3d8f..1a25bfcfced6 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -38,7 +38,7 @@ pub struct AppState { selection_panel: re_selection_panel::SelectionPanel, time_panel: re_time_panel::TimePanel, - blueprint_panel: re_time_panel::TimePanel, + blueprint_time_panel: re_time_panel::TimePanel, #[serde(skip)] blueprint_tree: re_blueprint_tree::BlueprintTree, @@ -86,7 +86,7 @@ impl Default for AppState { blueprint_cfg: Default::default(), selection_panel: Default::default(), time_panel: Default::default(), - blueprint_panel: re_time_panel::TimePanel::new_blueprint_panel(), + blueprint_time_panel: re_time_panel::TimePanel::new_blueprint_panel(), blueprint_tree: Default::default(), welcome_screen: Default::default(), datastore_ui: Default::default(), @@ -163,7 +163,7 @@ impl AppState { blueprint_cfg, selection_panel, time_panel, - blueprint_panel, + blueprint_time_panel, blueprint_tree, welcome_screen, datastore_ui, @@ -355,14 +355,46 @@ impl AppState { // if app_options.inspect_blueprint_timeline { - blueprint_panel.show_panel( + let blueprint_db = ctx.store_context.blueprint; + + let undo_state = self + .blueprint_undo_state + .entry(ctx.store_context.blueprint.store_id().clone()) + .or_default(); + + { + // Copy time from undo-state to the blueprint time control struct: + let mut time_ctrl = blueprint_cfg.time_ctrl.write(); + if let Some(redo_time) = undo_state.redo_time() { + time_ctrl + .set_play_state(blueprint_db.times_per_timeline(), PlayState::Paused); + time_ctrl.set_time(redo_time); + } else { + time_ctrl.set_play_state( + blueprint_db.times_per_timeline(), + PlayState::Following, + ); + } + } + + blueprint_time_panel.show_panel( &ctx, &viewport_ui.blueprint, - ctx.store_context.blueprint, + blueprint_db, blueprint_cfg, ui, PanelState::Expanded, ); + + { + // Apply changes to the blueprint time to the undo-state: + let time_ctrl = blueprint_cfg.time_ctrl.read(); + if time_ctrl.play_state() == PlayState::Following { + undo_state.redo_all(); + } else if let Some(time) = time_ctrl.time_int() { + undo_state.set_redo_time(time); + } + } } // diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index b7e0ca229e81..ae9ff3f3a562 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -56,6 +56,16 @@ impl BlueprintUndoState { } } + /// If set, everything after this time is in "redo-space" (futurum). + /// If `None`, there is no undo-buffer. + pub fn redo_time(&self) -> Option { + self.current_time + } + + pub fn set_redo_time(&mut self, time: TimeInt) { + self.current_time = Some(time); + } + pub fn undo(&mut self, blueprint_db: &EntityDb) { let time = self .current_time @@ -69,7 +79,7 @@ impl BlueprintUndoState { } } - pub fn redo(&mut self, _blueprint_db: &EntityDb) { + pub fn redo(&mut self) { if let Some(time) = self.current_time { self.current_time = self.inflection_points.range(time.inc()..).next().copied(); } else { @@ -78,6 +88,10 @@ impl BlueprintUndoState { } } + pub fn redo_all(&mut self) { + self.current_time = None; + } + /// Clears the "redo buffer". pub fn clear_redo(&mut self, blueprint_db: &mut EntityDb) { re_tracing::profile_function!(); From 5874d4796d8964c699c1d8aee0ce0f839df0efdb Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 1 Dec 2024 18:44:50 +0100 Subject: [PATCH 13/20] Better sync blueprint time panel and the undo state --- crates/viewer/re_viewer/src/app.rs | 31 ++++----------------- crates/viewer/re_viewer_context/src/undo.rs | 4 +-- 2 files changed, 7 insertions(+), 28 deletions(-) diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index 6a2a255bacb1..c7939d80a07f 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -517,28 +517,11 @@ impl App { SystemCommand::UpdateBlueprint(blueprint_id, updates) => { let blueprint_db = store_hub.entity_db_mut(&blueprint_id); - if self.state.app_options.inspect_blueprint_timeline { - // We may we viewing a historical blueprint, and doing an edit based on that. - // We therefor throw away everything after the currently viewed time (like an undo) - let last_kept_event_time = - self.state.blueprint_query_for_viewer(blueprint_db).at(); - let first_dropped_event_time = last_kept_event_time.inc(); - blueprint_db.drop_time_range( - &re_viewer_context::blueprint_timeline(), - re_log_types::ResolvedTimeRange::new( - first_dropped_event_time, - re_chunk::TimeInt::MAX, - ), - ); - } else { - let undo_state = self - .state - .blueprint_undo_state - .entry(blueprint_id) - .or_default(); - - undo_state.clear_redo(blueprint_db); - } + self.state + .blueprint_undo_state + .entry(blueprint_id) + .or_default() + .clear_redo_buffer(blueprint_db); for chunk in updates { match blueprint_db.add_chunk(&Arc::new(chunk)) { @@ -548,10 +531,6 @@ impl App { } } } - - // If we inspect the timeline, make sure we show the latest state: - let mut time_ctrl = self.state.blueprint_cfg.time_ctrl.write(); - time_ctrl.set_play_state(blueprint_db.times_per_timeline(), PlayState::Following); } SystemCommand::UndoBlueprint { blueprint_id } => { let blueprint_db = store_hub.entity_db_mut(&blueprint_id); diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index ae9ff3f3a562..e18330fb9e6d 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -92,8 +92,8 @@ impl BlueprintUndoState { self.current_time = None; } - /// Clears the "redo buffer". - pub fn clear_redo(&mut self, blueprint_db: &mut EntityDb) { + /// After calling this, there is no way to redo what was once undone. + pub fn clear_redo_buffer(&mut self, blueprint_db: &mut EntityDb) { re_tracing::profile_function!(); if let Some(last_kept_event_time) = self.current_time.take() { From 19c9b03da1b81668720604ee150238cf8f3e96f3 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 10:19:17 +0100 Subject: [PATCH 14/20] Give the blueprint timepanel a blueish tint --- Cargo.lock | 7 +++++++ Cargo.toml | 1 + crates/viewer/re_time_panel/src/lib.rs | 4 ++-- crates/viewer/re_viewer/src/app_state.rs | 5 ++++- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8fe39fed0966..48591f0ad2fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1398,6 +1398,12 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "color-hex" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ecdffb913a326b6c642290a0d0ec8e8d6597291acdc07cc4c9cb4b3635d44cf9" + [[package]] name = "colorchoice" version = "1.0.3" @@ -1920,6 +1926,7 @@ version = "0.29.1" source = "git+https://github.com/emilk/egui.git?rev=84cc1572b175d49a64f1b323a6d7e56b1f1fba66#84cc1572b175d49a64f1b323a6d7e56b1f1fba66" dependencies = [ "bytemuck", + "color-hex", "emath", "serde", ] diff --git a/Cargo.toml b/Cargo.toml index 5e63e6e0c5d2..aaa142bea4e0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -134,6 +134,7 @@ eframe = { version = "0.29.1", default-features = false, features = [ ] } egui = { version = "0.29.1", features = [ "callstack", + "color-hex", "log", "puffin", "rayon", diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index 31db40351a11..8cb1807522a6 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -157,6 +157,7 @@ impl TimePanel { } } + #[allow(clippy::too_many_arguments)] pub fn show_panel( &mut self, ctx: &ViewerContext<'_>, @@ -165,6 +166,7 @@ impl TimePanel { rec_cfg: &RecordingConfig, ui: &mut egui::Ui, state: PanelState, + mut panel_frame: egui::Frame, ) { if state.is_hidden() { return; @@ -181,8 +183,6 @@ impl TimePanel { // etc.) let screen_header_height = ui.cursor().top(); - let mut panel_frame = DesignTokens::bottom_panel_frame(); - if state.is_expanded() { // Since we use scroll bars we want to fill the whole vertical space downwards: panel_frame.inner_margin.bottom = 0.0; diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 1a25bfcfced6..11474cea6f92 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -6,7 +6,7 @@ use re_entity_db::EntityDb; use re_log_types::{LogMsg, ResolvedTimeRangeF, StoreId}; use re_smart_channel::ReceiveSet; use re_types::blueprint::components::PanelState; -use re_ui::ContextExt as _; +use re_ui::{ContextExt as _, DesignTokens}; use re_viewer_context::{ AppOptions, ApplicationSelectionState, BlueprintUndoState, CommandSender, ComponentUiRegistry, PlayState, RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, @@ -384,6 +384,8 @@ impl AppState { blueprint_cfg, ui, PanelState::Expanded, + // Give the blueprint time panel a distinct color from the normal time panel: + DesignTokens::bottom_panel_frame().fill(egui::hex_color!("#141326")), ); { @@ -408,6 +410,7 @@ impl AppState { ctx.rec_cfg, ui, app_blueprint.time_panel_state(), + DesignTokens::bottom_panel_frame(), ); // From 6c9cb630b31958c973ff7a6085cadec4f56664c9 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 10:33:47 +0100 Subject: [PATCH 15/20] fix typo Co-authored-by: Clement Rey --- crates/viewer/re_viewer/src/app_state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 11474cea6f92..36d6d5cebe78 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -33,7 +33,7 @@ pub struct AppState { recording_configs: HashMap, pub blueprint_cfg: RecordingConfig, - /// Maps blueperint id to the current undo state for it. + /// Maps blueprint id to the current undo state for it. pub blueprint_undo_state: HashMap, selection_panel: re_selection_panel::SelectionPanel, From 2d45faca0a42f63587da3e7aca2338a640548ebb Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 10:43:32 +0100 Subject: [PATCH 16/20] Improve logging --- crates/viewer/re_viewer/src/app.rs | 10 ++++++++-- crates/viewer/re_viewer_context/src/undo.rs | 6 +++++- crates/viewer/re_viewport_blueprint/src/lib.rs | 1 + 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index c7939d80a07f..6ac5a3997d91 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use itertools::Itertools as _; use re_build_info::CrateVersion; use re_data_source::{DataSource, FileContents}; use re_entity_db::entity_db::EntityDb; @@ -514,7 +515,12 @@ impl App { store_hub.clear_active_blueprint(); egui_ctx.request_repaint(); // Many changes take a frame delay to show up. } - SystemCommand::UpdateBlueprint(blueprint_id, updates) => { + SystemCommand::UpdateBlueprint(blueprint_id, chunks) => { + re_log::trace!( + "Update blueprint entities: {}", + chunks.iter().map(|c| c.entity_path()).join(", ") + ); + let blueprint_db = store_hub.entity_db_mut(&blueprint_id); self.state @@ -523,7 +529,7 @@ impl App { .or_default() .clear_redo_buffer(blueprint_db); - for chunk in updates { + for chunk in chunks { match blueprint_db.add_chunk(&Arc::new(chunk)) { Ok(_store_events) => {} Err(err) => { diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs index e18330fb9e6d..e63c09c2967d 100644 --- a/crates/viewer/re_viewer_context/src/undo.rs +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -72,6 +72,7 @@ impl BlueprintUndoState { .unwrap_or_else(|| max_blueprint_time(blueprint_db)); if let Some(previous) = self.inflection_points.range(..time).next_back().copied() { + re_log::trace!("Undo"); self.current_time = Some(previous); } else { // nothing to undo to @@ -81,6 +82,7 @@ impl BlueprintUndoState { pub fn redo(&mut self) { if let Some(time) = self.current_time { + re_log::trace!("Redo"); self.current_time = self.inflection_points.range(time.inc()..).next().copied(); } else { // If we have no time, we're at latest, and have nothing to redo @@ -101,10 +103,12 @@ impl BlueprintUndoState { TimeInt::new_temporal(last_kept_event_time.as_i64().saturating_add(1)); // Drop everything after the current timeline time - blueprint_db.drop_time_range( + let events = blueprint_db.drop_time_range( &blueprint_timeline(), ResolvedTimeRange::new(first_dropped_event_time, re_chunk::TimeInt::MAX), ); + + re_log::trace!("{} chunks affected when clearing redo buffer", events.len()); } } diff --git a/crates/viewer/re_viewport_blueprint/src/lib.rs b/crates/viewer/re_viewport_blueprint/src/lib.rs index 33196fa33b0a..8a5b6154de07 100644 --- a/crates/viewer/re_viewport_blueprint/src/lib.rs +++ b/crates/viewer/re_viewport_blueprint/src/lib.rs @@ -18,6 +18,7 @@ pub use view_properties::{entity_path_for_view_property, ViewProperty, ViewPrope pub use viewport_blueprint::ViewportBlueprint; pub use viewport_command::ViewportCommand; +/// The entity path of the viewport blueprint in the blueprint store. pub const VIEWPORT_PATH: &str = "viewport"; /// Converts a [`re_types_blueprint::blueprint::components::ContainerKind`] into a [`egui_tiles::ContainerKind`]. From 5aa8eee3c35163f03ffc2c1ebbe978e42badedfa Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 10:46:41 +0100 Subject: [PATCH 17/20] Make use of the `ctx.save_blueprint_archetype` helper --- .../re_viewer_context/src/command_sender.rs | 3 ++ .../re_viewport_blueprint/src/container.rs | 34 +++---------------- .../re_viewport_blueprint/src/space_view.rs | 20 +++-------- 3 files changed, 12 insertions(+), 45 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/command_sender.rs b/crates/viewer/re_viewer_context/src/command_sender.rs index a0abe954bcbe..f17cb16f67a0 100644 --- a/crates/viewer/re_viewer_context/src/command_sender.rs +++ b/crates/viewer/re_viewer_context/src/command_sender.rs @@ -47,6 +47,9 @@ pub enum SystemCommand { /// The [`StoreId`] should generally be the currently selected blueprint /// but is tracked manually to ensure self-consistency if the blueprint /// is both modified and changed in the same frame. + /// + /// Instead of using this directly, consider using + /// [`ViewerContext::save_blueprint_archetype`] or similar. UpdateBlueprint(StoreId, Vec), UndoBlueprint { diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 52e583f3b1a6..ad2fe7c77375 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -171,8 +171,6 @@ impl ContainerBlueprint { /// 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 = ctx.store_context.blueprint_timepoint_for_writes(); - let Self { id, container_kind, @@ -215,17 +213,7 @@ impl ContainerBlueprint { ctx.save_empty_blueprint_component::(&id.as_entity_path()); } - if let Some(chunk) = Chunk::builder(id.as_entity_path()) - .with_archetype(RowId::new(), timepoint, &arch) - .build() - .warn_on_err_once("Failed to create container blueprint.") - { - ctx.command_sender - .send_system(SystemCommand::UpdateBlueprint( - ctx.store_context.blueprint.store_id().clone(), - vec![chunk], - )); - } + ctx.save_blueprint_archetype(&id.as_entity_path(), &arch); } /// Creates a new [`ContainerBlueprint`] from the given [`egui_tiles::Container`]. @@ -377,22 +365,10 @@ impl ContainerBlueprint { pub fn clear(&self, ctx: &ViewerContext<'_>) { // We can't delete the entity, because we need to support undo. // TODO(#8249): configure blueprint GC to remove this entity if all that remains is the recursive clear. - let timepoint = ctx.store_context.blueprint_timepoint_for_writes(); - - let chunk = Chunk::builder(self.entity_path()) - .with_archetype( - RowId::new(), - timepoint.clone(), - &re_types::archetypes::Clear::recursive(), - ) - .build() - .expect("Failed to serialize Clear component !?"); - - ctx.command_sender - .send_system(SystemCommand::UpdateBlueprint( - ctx.store_context.blueprint.store_id().clone(), - vec![chunk], - )); + ctx.save_blueprint_archetype( + &self.entity_path(), + &re_types::archetypes::Clear::recursive(), + ); } pub fn to_tile(&self) -> egui_tiles::Tile { diff --git a/crates/viewer/re_viewport_blueprint/src/space_view.rs b/crates/viewer/re_viewport_blueprint/src/space_view.rs index 1d2cad6ec5cf..4ebdd42cd85e 100644 --- a/crates/viewer/re_viewport_blueprint/src/space_view.rs +++ b/crates/viewer/re_viewport_blueprint/src/space_view.rs @@ -312,22 +312,10 @@ impl SpaceViewBlueprint { pub fn clear(&self, ctx: &ViewerContext<'_>) { // We can't delete the entity, because we need to support undo. // TODO(#8249): configure blueprint GC to remove this entity if all that remains is the recursive clear. - let timepoint = ctx.store_context.blueprint_timepoint_for_writes(); - - let chunk = Chunk::builder(self.entity_path()) - .with_archetype( - RowId::new(), - timepoint.clone(), - &re_types::archetypes::Clear::recursive(), - ) - .build() - .expect("Failed to serialize Clear component !?"); - - ctx.command_sender - .send_system(SystemCommand::UpdateBlueprint( - ctx.store_context.blueprint.store_id().clone(), - vec![chunk], - )); + ctx.save_blueprint_archetype( + &self.entity_path(), + &re_types::archetypes::Clear::recursive(), + ); } #[inline] From 0d6b3c6364fb7b8823b08d0f01759e25a0cd6955 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 10:50:15 +0100 Subject: [PATCH 18/20] Remove unused imports --- crates/viewer/re_viewport_blueprint/src/container.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index ad2fe7c77375..1d3161437544 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -1,18 +1,14 @@ use ahash::HashMap; use egui_tiles::TileId; -use re_chunk::{Chunk, LatestAtQuery, RowId}; +use re_chunk::LatestAtQuery; use re_entity_db::EntityDb; -use re_log::ResultExt; use re_log_types::EntityPath; use re_types::components::Name; use re_types::{blueprint::components::Visible, Archetype as _}; use re_types_blueprint::blueprint::archetypes as blueprint_archetypes; use re_types_blueprint::blueprint::components::{ContainerKind, GridColumns}; -use re_viewer_context::{ - ContainerId, Contents, ContentsName, SpaceViewId, SystemCommand, SystemCommandSender as _, - ViewerContext, -}; +use re_viewer_context::{ContainerId, Contents, ContentsName, SpaceViewId, ViewerContext}; /// The native version of a [`re_types_blueprint::blueprint::archetypes::ContainerBlueprint`]. /// From d7d250c6920162d79cf951efa845cb4342cbf235 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 11:01:41 +0100 Subject: [PATCH 19/20] Fix doclink --- crates/viewer/re_viewer_context/src/command_sender.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_viewer_context/src/command_sender.rs b/crates/viewer/re_viewer_context/src/command_sender.rs index f17cb16f67a0..1f0fb34acea5 100644 --- a/crates/viewer/re_viewer_context/src/command_sender.rs +++ b/crates/viewer/re_viewer_context/src/command_sender.rs @@ -49,7 +49,7 @@ pub enum SystemCommand { /// is both modified and changed in the same frame. /// /// Instead of using this directly, consider using - /// [`ViewerContext::save_blueprint_archetype`] or similar. + /// [`crate::ViewerContext::save_blueprint_archetype`] or similar. UpdateBlueprint(StoreId, Vec), UndoBlueprint { From 2e6be7616816c01948ee7c06423b24d2194f42cf Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 11:14:34 +0100 Subject: [PATCH 20/20] Fix hovering entities in the blueprint time panel --- crates/viewer/re_data_ui/src/item_ui.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_data_ui/src/item_ui.rs b/crates/viewer/re_data_ui/src/item_ui.rs index 49a55035fd83..024f57baaf63 100644 --- a/crates/viewer/re_data_ui/src/item_ui.rs +++ b/crates/viewer/re_data_ui/src/item_ui.rs @@ -607,7 +607,7 @@ pub fn instance_hover_card_ui( instance_path: &InstancePath, include_subtree: bool, ) { - if !ctx.recording().is_known_entity(&instance_path.entity_path) { + if !db.is_known_entity(&instance_path.entity_path) { ui.label("Unknown entity."); return; } @@ -626,7 +626,7 @@ pub fn instance_hover_card_ui( // Then we can move the size view into `data_ui`. if instance_path.instance.is_all() { - if let Some(subtree) = ctx.recording().tree().subtree(&instance_path.entity_path) { + if let Some(subtree) = db.tree().subtree(&instance_path.entity_path) { entity_tree_stats_ui(ui, &query.timeline(), db, subtree, include_subtree); } } else {