From c9a1e42e9ca614a56b73fc7b8f1268336b1753ae Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 2 Dec 2024 11:39:20 +0100 Subject: [PATCH] Add Undo/Redo support in the viewer (#7546) ### What * Closes https://github.com/rerun-io/rerun/issues/3135 * Proceeded by https://github.com/rerun-io/rerun/pull/7602 * Proceeded by https://github.com/rerun-io/rerun/pull/7603 * Proceeded by https://github.com/rerun-io/rerun/pull/8241 * New issue: https://github.com/rerun-io/rerun/issues/8249 This PR implements Undo and Redo for any edit to the active blueprint. https://github.com/user-attachments/assets/05018729-f01e-42f4-a84f-b48dbf31b060 ### Implementation This implements undo/redo by editing the "time cursor" for the blueprint timeline. Undo moves it backwards, redo forwards. When doing an action, all redo history is erased from the store with a new `ChunkStore::drop_time_range` function. ### Known shortcomings * Undo doesn't work when the blueprint streams panel is open ### 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 examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7546?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7546?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 * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/7546) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --------- Co-authored-by: Clement Rey --- Cargo.lock | 7 + Cargo.toml | 1 + crates/store/re_entity_db/src/entity_db.rs | 17 +- crates/store/re_entity_db/src/lib.rs | 2 +- crates/viewer/re_data_ui/src/item_ui.rs | 4 +- crates/viewer/re_time_panel/src/lib.rs | 4 +- crates/viewer/re_ui/src/command.rs | 17 +- crates/viewer/re_viewer/src/app.rs | 83 ++++++--- crates/viewer/re_viewer/src/app_state.rs | 80 +++++++-- crates/viewer/re_viewer/src/ui/rerun_menu.rs | 3 + .../re_viewer_context/src/command_sender.rs | 10 ++ crates/viewer/re_viewer_context/src/lib.rs | 2 + .../viewer/re_viewer_context/src/store_hub.rs | 40 +++-- .../re_viewer_context/src/test_context.rs | 2 + crates/viewer/re_viewer_context/src/undo.rs | 161 ++++++++++++++++++ .../re_viewport_blueprint/src/container.rs | 51 +++--- .../viewer/re_viewport_blueprint/src/lib.rs | 1 + .../re_viewport_blueprint/src/space_view.rs | 10 +- .../src/viewport_blueprint.rs | 26 ++- 19 files changed, 412 insertions(+), 109 deletions(-) create mode 100644 crates/viewer/re_viewer_context/src/undo.rs diff --git a/Cargo.lock b/Cargo.lock index e717241ce88c..e04a639c7f06 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/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_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 { 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_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 5fa0cb05ebdc..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; @@ -10,9 +11,9 @@ 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; @@ -514,24 +515,21 @@ 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); - 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 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, - ), - ); - } + self.state + .blueprint_undo_state + .entry(blueprint_id) + .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) => { @@ -539,11 +537,23 @@ 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); + self.state + .blueprint_undo_state + .entry(blueprint_id) + .or_default() + .undo(blueprint_db); + } + SystemCommand::RedoBlueprint { blueprint_id } => { + self.state + .blueprint_undo_state + .entry(blueprint_id) + .or_default() + .redo(); + } + 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 +720,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); @@ -1665,7 +1690,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() { @@ -1793,7 +1818,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(); @@ -1820,9 +1845,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 89d82a0e92b4..36d6d5cebe78 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -6,9 +6,9 @@ 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::{ - blueprint_timeline, AppOptions, ApplicationSelectionState, CommandSender, ComponentUiRegistry, + AppOptions, ApplicationSelectionState, BlueprintUndoState, CommandSender, ComponentUiRegistry, PlayState, RecordingConfig, SpaceViewClassExt as _, SpaceViewClassRegistry, StoreContext, StoreHub, SystemCommandSender as _, ViewStates, ViewerContext, }; @@ -16,8 +16,10 @@ use re_viewport::ViewportUi; 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, + ui::{recordings_panel_ui, settings_screen_ui}, +}; const WATERMARK: bool = false; // Nice for recording media material @@ -31,9 +33,12 @@ pub struct AppState { recording_configs: HashMap, pub blueprint_cfg: RecordingConfig, + /// Maps blueprint 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, + blueprint_time_panel: re_time_panel::TimePanel, #[serde(skip)] blueprint_tree: re_blueprint_tree::BlueprintTree, @@ -77,10 +82,11 @@ 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(), - 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(), @@ -148,15 +154,16 @@ 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, - blueprint_panel, + blueprint_time_panel, blueprint_tree, welcome_screen, datastore_ui, @@ -170,6 +177,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); + let viewport_blueprint = ViewportBlueprint::try_from_db(store_context.blueprint, &blueprint_query); let viewport_ui = ViewportUi::new(viewport_blueprint); @@ -343,14 +355,48 @@ 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, + // Give the blueprint time panel a distinct color from the normal time panel: + DesignTokens::bottom_panel_frame().fill(egui::hex_color!("#141326")), ); + + { + // 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); + } + } } // @@ -364,6 +410,7 @@ impl AppState { ctx.rec_cfg, ui, app_blueprint.time_panel_state(), + DesignTokens::bottom_panel_frame(), ); // @@ -491,6 +538,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 @@ -498,17 +548,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/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_context/src/command_sender.rs b/crates/viewer/re_viewer_context/src/command_sender.rs index 0219a5d31230..1f0fb34acea5 100644 --- a/crates/viewer/re_viewer_context/src/command_sender.rs +++ b/crates/viewer/re_viewer_context/src/command_sender.rs @@ -47,8 +47,18 @@ 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 + /// [`crate::ViewerContext::save_blueprint_archetype`] or similar. 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/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_context/src/store_hub.rs b/crates/viewer/re_viewer_context/src/store_hub.rs index 192990f46ae2..977ac4f3e167 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,12 +699,29 @@ 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(); - if let Some(caches) = self.caches_per_recording.get_mut(blueprint_id) { - caches.on_store_events(&store_events); + 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 { + // TODO(#8249): configure blueprint GC to remove an entity if all that remains of it is a recursive clear + target: GarbageCollectionTarget::Everything, + 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, + }); + 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/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 3e207bb77372..c2c75e74babb 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -185,6 +185,8 @@ impl TestContext { | SystemCommand::ClearAndGenerateBlueprint | SystemCommand::ActivateRecording(_) | SystemCommand::CloseStore(_) + | SystemCommand::UndoBlueprint { .. } + | SystemCommand::RedoBlueprint { .. } | SystemCommand::CloseAllRecordings => handled = false, #[cfg(debug_assertions)] diff --git a/crates/viewer/re_viewer_context/src/undo.rs b/crates/viewer/re_viewer_context/src/undo.rs new file mode 100644 index 000000000000..e63c09c2967d --- /dev/null +++ b/crates/viewer/re_viewer_context/src/undo.rs @@ -0,0 +1,161 @@ +use std::collections::BTreeSet; + +use re_chunk::{LatestAtQuery, TimeInt}; +use re_entity_db::EntityDb; +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. +/// 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()) + } + + /// 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) + } else { + Self::default_query() + } + } + + /// 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 + .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 + re_log::debug!("Nothing to undo"); + } + } + + 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 + re_log::debug!("Nothing to redo"); + } + } + + pub fn redo_all(&mut self) { + self.current_time = None; + } + + /// 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() { + let first_dropped_event_time = + TimeInt::new_temporal(last_kept_event_time.as_i64().saturating_add(1)); + + // Drop everything after the current timeline time + 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()); + } + } + + // 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! + + // 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; + } + } + } +} + +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_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index b5181292d8d8..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`]. /// @@ -146,6 +142,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`]. @@ -153,8 +167,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, @@ -197,17 +209,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`]. @@ -356,12 +358,13 @@ 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(), - )); + // 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. + 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/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`]. diff --git a/crates/viewer/re_viewport_blueprint/src/space_view.rs b/crates/viewer/re_viewport_blueprint/src/space_view.rs index 5962cbe48886..4ebdd42cd85e 100644 --- a/crates/viewer/re_viewport_blueprint/src/space_view.rs +++ b/crates/viewer/re_viewport_blueprint/src/space_view.rs @@ -310,10 +310,12 @@ 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(), - )); + // 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. + ctx.save_blueprint_archetype( + &self.entity_path(), + &re_types::archetypes::Clear::recursive(), + ); } #[inline] diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index 58f3e036e45b..239865b24404 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -1,19 +1,25 @@ -use std::sync::atomic::{AtomicBool, Ordering}; -use std::{collections::BTreeMap, sync::Arc}; +use std::{ + collections::BTreeMap, + sync::{ + atomic::{AtomicBool, Ordering}, + Arc, + }, +}; use ahash::HashMap; use egui_tiles::{SimplificationOptions, TileId}; use nohash_hasher::IntSet; use parking_lot::Mutex; -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_blueprint::blueprint::archetypes as blueprint_archetypes; -use re_types_blueprint::blueprint::components::{ - AutoLayout, AutoSpaceViews, RootContainer, SpaceViewMaximized, +use re_types::{ + blueprint::components::ViewerRecommendationHash, Archetype as _, SpaceViewClassIdentifier, +}; +use re_types_blueprint::blueprint::{ + archetypes as blueprint_archetypes, + components::{AutoLayout, AutoSpaceViews, RootContainer, SpaceViewMaximized}, }; use re_viewer_context::{ blueprint_id_to_tile_id, ContainerId, Contents, Item, SpaceViewId, ViewerContext, @@ -835,8 +841,8 @@ impl ViewportBlueprint { } } - // 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() { @@ -872,8 +878,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()); } }