From 1f4c61a6ddc6132465d7774252025ff692a12392 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Thu, 31 Oct 2024 21:34:35 +0100 Subject: [PATCH] implement consistent open/import/log_file behavior for all cases --- Cargo.lock | 2 + crates/store/re_data_loader/Cargo.toml | 1 + crates/store/re_data_loader/src/lib.rs | 37 +++++++--- crates/store/re_data_loader/src/load_file.rs | 49 ++++++++----- .../re_data_loader/src/loader_archetype.rs | 2 +- .../re_data_loader/src/loader_external.rs | 4 +- crates/store/re_data_loader/src/loader_rrd.rs | 4 +- .../store/re_data_source/src/data_source.rs | 3 + crates/store/re_log_types/src/lib.rs | 31 +++++++++ crates/top/re_sdk/src/recording_stream.rs | 1 + crates/viewer/re_viewer/Cargo.toml | 1 + crates/viewer/re_viewer/src/app.rs | 68 ++++++++++++++----- examples/rust/custom_data_loader/src/main.rs | 4 +- 13 files changed, 159 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0e0ad006872..aa44d532279f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5376,6 +5376,7 @@ dependencies = [ "re_types", "tempfile", "thiserror", + "uuid", "walkdir", ] @@ -6415,6 +6416,7 @@ dependencies = [ "strum", "strum_macros", "thiserror", + "uuid", "wasm-bindgen", "wasm-bindgen-futures", "web-sys", diff --git a/crates/store/re_data_loader/Cargo.toml b/crates/store/re_data_loader/Cargo.toml index 1a85d3067548..80ba230cc383 100644 --- a/crates/store/re_data_loader/Cargo.toml +++ b/crates/store/re_data_loader/Cargo.toml @@ -43,6 +43,7 @@ once_cell.workspace = true parking_lot.workspace = true rayon.workspace = true thiserror.workspace = true +uuid.workspace = true walkdir.workspace = true [target.'cfg(not(any(target_arch = "wasm32")))'.dependencies] diff --git a/crates/store/re_data_loader/src/lib.rs b/crates/store/re_data_loader/src/lib.rs index 73c987d01848..16b8a3c8de7e 100644 --- a/crates/store/re_data_loader/src/lib.rs +++ b/crates/store/re_data_loader/src/lib.rs @@ -64,6 +64,12 @@ pub struct DataLoaderSettings { /// The [`re_log_types::StoreId`] that is currently opened in the viewer, if any. pub opened_store_id: Option, + /// Whether `SetStoreInfo`s should be sent, regardless of the surrounding context. + /// + /// Only useful when creating a recording just-in-time directly in the viewer (which is what + /// happens when importing things into the welcome screen). + pub force_store_info: bool, + /// What should the logged entity paths be prefixed with? pub entity_path_prefix: Option, @@ -79,6 +85,7 @@ impl DataLoaderSettings { opened_application_id: Default::default(), store_id: store_id.into(), opened_store_id: Default::default(), + force_store_info: false, entity_path_prefix: Default::default(), timepoint: Default::default(), } @@ -91,6 +98,7 @@ impl DataLoaderSettings { opened_application_id, store_id, opened_store_id, + force_store_info: _, entity_path_prefix, timepoint, } = self; @@ -150,6 +158,8 @@ impl DataLoaderSettings { } } +pub type DataLoaderName = String; + /// A [`DataLoader`] loads data from a file path and/or a file's contents. /// /// Files can be loaded in 3 different ways: @@ -205,8 +215,8 @@ impl DataLoaderSettings { pub trait DataLoader: Send + Sync { /// Name of the [`DataLoader`]. /// - /// Doesn't need to be unique. - fn name(&self) -> String; + /// Should be globally unique. + fn name(&self) -> DataLoaderName; /// Loads data from a file on the local filesystem and sends it to `tx`. /// @@ -314,20 +324,31 @@ impl DataLoaderError { /// most convenient for them, whether it is raw components, arrow chunks or even /// full-on [`LogMsg`]s. pub enum LoadedData { - Chunk(re_log_types::StoreId, Chunk), - ArrowMsg(re_log_types::StoreId, ArrowMsg), - LogMsg(LogMsg), + Chunk(DataLoaderName, re_log_types::StoreId, Chunk), + ArrowMsg(DataLoaderName, re_log_types::StoreId, ArrowMsg), + LogMsg(DataLoaderName, LogMsg), } impl LoadedData { + /// Returns the name of the [`DataLoader`] that generated this data. + #[inline] + pub fn data_loader_name(&self) -> &DataLoaderName { + match self { + Self::Chunk(name, ..) | Self::ArrowMsg(name, ..) | Self::LogMsg(name, ..) => name, + } + } + /// Pack the data into a [`LogMsg`]. + #[inline] pub fn into_log_msg(self) -> ChunkResult { match self { - Self::Chunk(store_id, chunk) => Ok(LogMsg::ArrowMsg(store_id, chunk.to_arrow_msg()?)), + Self::Chunk(_name, store_id, chunk) => { + Ok(LogMsg::ArrowMsg(store_id, chunk.to_arrow_msg()?)) + } - Self::ArrowMsg(store_id, msg) => Ok(LogMsg::ArrowMsg(store_id, msg)), + Self::ArrowMsg(_name, store_id, msg) => Ok(LogMsg::ArrowMsg(store_id, msg)), - Self::LogMsg(msg) => Ok(msg), + Self::LogMsg(_name, msg) => Ok(msg), } } } diff --git a/crates/store/re_data_loader/src/load_file.rs b/crates/store/re_data_loader/src/load_file.rs index 3ed813a002f3..4ee48c1d14c4 100644 --- a/crates/store/re_data_loader/src/load_file.rs +++ b/crates/store/re_data_loader/src/load_file.rs @@ -4,7 +4,7 @@ use ahash::{HashMap, HashMapExt}; use re_log_types::{FileSource, LogMsg}; use re_smart_channel::Sender; -use crate::{DataLoaderError, LoadedData}; +use crate::{DataLoader, DataLoaderError, LoadedData, RrdLoader}; // --- @@ -37,7 +37,7 @@ pub fn load_from_path( let rx = load(settings, path, None)?; - send(settings.clone(), file_source, path.to_owned(), rx, tx); + send(settings.clone(), file_source, rx, tx); Ok(()) } @@ -64,7 +64,7 @@ pub fn load_from_file_contents( let data = load(settings, filepath, Some(contents))?; - send(settings.clone(), file_source, filepath.to_owned(), data, tx); + send(settings.clone(), file_source, data, tx); Ok(()) } @@ -73,21 +73,20 @@ pub fn load_from_file_contents( /// Prepares an adequate [`re_log_types::StoreInfo`] [`LogMsg`] given the input. pub(crate) fn prepare_store_info( + application_id: re_log_types::ApplicationId, store_id: &re_log_types::StoreId, file_source: FileSource, - path: &std::path::Path, ) -> LogMsg { - re_tracing::profile_function!(path.display().to_string()); + re_tracing::profile_function!(); use re_log_types::SetStoreInfo; - let app_id = re_log_types::ApplicationId(path.display().to_string()); let store_source = re_log_types::StoreSource::File { file_source }; LogMsg::SetStoreInfo(SetStoreInfo { row_id: *re_chunk::RowId::new(), info: re_log_types::StoreInfo { - application_id: app_id.clone(), + application_id, store_id: store_id.clone(), cloned_from: None, is_official_example: false, @@ -263,14 +262,19 @@ pub(crate) fn load( pub(crate) fn send( settings: crate::DataLoaderSettings, file_source: FileSource, - path: std::path::PathBuf, rx_loader: std::sync::mpsc::Receiver, tx: &Sender, ) { spawn({ re_tracing::profile_function!(); - let mut store_info_tracker: HashMap = HashMap::new(); + #[derive(Default, Debug)] + struct Tracked { + is_rrd_or_rbl: bool, + already_has_store_info: bool, + } + + let mut store_info_tracker: HashMap = HashMap::new(); let tx = tx.clone(); move || { @@ -280,6 +284,7 @@ pub(crate) fn send( // poll the channel in any case so as to make sure that the data producer // doesn't get stuck. for data in rx_loader { + let data_loader_name = data.data_loader_name().clone(); let msg = match data.into_log_msg() { Ok(msg) => { let store_info = match &msg { @@ -293,7 +298,10 @@ pub(crate) fn send( }; if let Some((store_id, store_info_created)) = store_info { - *store_info_tracker.entry(store_id).or_default() |= store_info_created; + let tracked = store_info_tracker.entry(store_id).or_default(); + tracked.is_rrd_or_rbl = + *data_loader_name == RrdLoader::name(&RrdLoader); + tracked.already_has_store_info |= store_info_created; } msg @@ -306,16 +314,25 @@ pub(crate) fn send( tx.send(msg).ok(); } - for (store_id, store_info_already_created) in store_info_tracker { + for (store_id, tracked) in store_info_tracker { let is_a_preexisting_recording = Some(&store_id) == settings.opened_store_id.as_ref(); - if store_info_already_created || is_a_preexisting_recording { - continue; - } + // Never try to send custom store info for RRDs and RBLs, they always have their own, and + // it's always right. + let should_force_store_info = !tracked.is_rrd_or_rbl && settings.force_store_info; + + let should_send_new_store_info = should_force_store_info + || (!tracked.already_has_store_info && !is_a_preexisting_recording); - let store_info = prepare_store_info(&store_id, file_source.clone(), &path); - tx.send(store_info).ok(); + if should_send_new_store_info { + let app_id = settings + .opened_application_id + .clone() + .unwrap_or_else(|| uuid::Uuid::new_v4().to_string().into()); + let store_info = prepare_store_info(app_id, &store_id, file_source.clone()); + tx.send(store_info).ok(); + } } tx.quit(None).ok(); diff --git a/crates/store/re_data_loader/src/loader_archetype.rs b/crates/store/re_data_loader/src/loader_archetype.rs index 39dfecd12231..041541823a08 100644 --- a/crates/store/re_data_loader/src/loader_archetype.rs +++ b/crates/store/re_data_loader/src/loader_archetype.rs @@ -138,7 +138,7 @@ impl DataLoader for ArchetypeLoader { .clone() .unwrap_or_else(|| settings.store_id.clone()); for row in rows { - let data = LoadedData::Chunk(store_id.clone(), row); + let data = LoadedData::Chunk(Self::name(&Self), store_id.clone(), row); if tx.send(data).is_err() { break; // The other end has decided to hang up, not our problem. } diff --git a/crates/store/re_data_loader/src/loader_external.rs b/crates/store/re_data_loader/src/loader_external.rs index 539876ba55d5..be4dacea5e61 100644 --- a/crates/store/re_data_loader/src/loader_external.rs +++ b/crates/store/re_data_loader/src/loader_external.rs @@ -6,7 +6,7 @@ use std::{ use ahash::HashMap; use once_cell::sync::Lazy; -use crate::LoadedData; +use crate::{DataLoader, LoadedData}; // --- @@ -321,7 +321,7 @@ fn decode_and_stream( } }; - let data = LoadedData::LogMsg(msg); + let data = LoadedData::LogMsg(ExternalLoader::name(&ExternalLoader), msg); if tx.send(data).is_err() { break; // The other end has decided to hang up, not our problem. } diff --git a/crates/store/re_data_loader/src/loader_rrd.rs b/crates/store/re_data_loader/src/loader_rrd.rs index 54d7d38d2540..e644e185a432 100644 --- a/crates/store/re_data_loader/src/loader_rrd.rs +++ b/crates/store/re_data_loader/src/loader_rrd.rs @@ -4,7 +4,7 @@ use re_log_encoding::decoder::Decoder; use crossbeam::channel::Receiver; use re_log_types::{ApplicationId, StoreId}; -use crate::LoadedData; +use crate::{DataLoader as _, LoadedData}; // --- @@ -192,7 +192,7 @@ fn decode_and_stream( msg }; - let data = LoadedData::LogMsg(msg); + let data = LoadedData::LogMsg(RrdLoader::name(&RrdLoader), msg); if tx.send(data).is_err() { break; // The other end has decided to hang up, not our problem. } diff --git a/crates/store/re_data_source/src/data_source.rs b/crates/store/re_data_source/src/data_source.rs index 6d9c76a77b54..b480a0e512a4 100644 --- a/crates/store/re_data_source/src/data_source.rs +++ b/crates/store/re_data_source/src/data_source.rs @@ -178,6 +178,7 @@ impl DataSource { let settings = re_data_loader::DataLoaderSettings { opened_application_id: file_source.recommended_application_id().cloned(), opened_store_id: file_source.recommended_recording_id().cloned(), + force_store_info: file_source.force_store_info(), ..re_data_loader::DataLoaderSettings::recommended(shared_store_id) }; re_data_loader::load_from_path(&settings, file_source, &path, &tx) @@ -206,6 +207,7 @@ impl DataSource { let settings = re_data_loader::DataLoaderSettings { opened_application_id: file_source.recommended_application_id().cloned(), opened_store_id: file_source.recommended_recording_id().cloned(), + force_store_info: file_source.force_store_info(), ..re_data_loader::DataLoaderSettings::recommended(shared_store_id) }; re_data_loader::load_from_file_contents( @@ -275,6 +277,7 @@ fn test_data_source_from_uri() { let file_source = FileSource::DragAndDrop { recommended_application_id: None, recommended_recording_id: None, + force_store_info: false, }; for uri in file { diff --git a/crates/store/re_log_types/src/lib.rs b/crates/store/re_log_types/src/lib.rs index 2a0abd433d5c..5f25a628e376 100644 --- a/crates/store/re_log_types/src/lib.rs +++ b/crates/store/re_log_types/src/lib.rs @@ -413,21 +413,39 @@ pub enum FileSource { DragAndDrop { /// The [`ApplicationId`] that the viewer heuristically recommends should be used when loading /// this data source, based on the surrounding context. + #[cfg_attr(feature = "serde", serde(skip))] recommended_application_id: Option, /// The [`StoreId`] that the viewer heuristically recommends should be used when loading /// this data source, based on the surrounding context. + #[cfg_attr(feature = "serde", serde(skip))] recommended_recording_id: Option, + + /// Whether `SetStoreInfo`s should be sent, regardless of the surrounding context. + /// + /// Only useful when creating a recording just-in-time directly in the viewer (which is what + /// happens when importing things into the welcome screen). + #[cfg_attr(feature = "serde", serde(skip))] + force_store_info: bool, }, FileDialog { /// The [`ApplicationId`] that the viewer heuristically recommends should be used when loading /// this data source, based on the surrounding context. + #[cfg_attr(feature = "serde", serde(skip))] recommended_application_id: Option, /// The [`StoreId`] that the viewer heuristically recommends should be used when loading /// this data source, based on the surrounding context. + #[cfg_attr(feature = "serde", serde(skip))] recommended_recording_id: Option, + + /// Whether `SetStoreInfo`s should be sent, regardless of the surrounding context. + /// + /// Only useful when creating a recording just-in-time directly in the viewer (which is what + /// happens when importing things into the welcome screen). + #[cfg_attr(feature = "serde", serde(skip))] + force_store_info: bool, }, Sdk, @@ -463,6 +481,19 @@ impl FileSource { Self::Cli | Self::Sdk => None, } } + + #[inline] + pub fn force_store_info(&self) -> bool { + match self { + Self::FileDialog { + force_store_info, .. + } + | Self::DragAndDrop { + force_store_info, .. + } => *force_store_info, + Self::Cli | Self::Sdk => false, + } + } } /// The source of a recording or blueprint. diff --git a/crates/top/re_sdk/src/recording_stream.rs b/crates/top/re_sdk/src/recording_stream.rs index 1e8abc28d071..19beb4c20a56 100644 --- a/crates/top/re_sdk/src/recording_stream.rs +++ b/crates/top/re_sdk/src/recording_stream.rs @@ -1259,6 +1259,7 @@ impl RecordingStream { opened_application_id: None, store_id: store_info.store_id.clone(), opened_store_id: None, + force_store_info: false, entity_path_prefix, timepoint: (!static_).then(|| { self.with(|inner| { diff --git a/crates/viewer/re_viewer/Cargo.toml b/crates/viewer/re_viewer/Cargo.toml index 1cff704f6e45..18af92ce86eb 100644 --- a/crates/viewer/re_viewer/Cargo.toml +++ b/crates/viewer/re_viewer/Cargo.toml @@ -120,6 +120,7 @@ serde = { workspace = true, features = ["derive"] } serde_json.workspace = true serde-wasm-bindgen.workspace = true thiserror.workspace = true +uuid.workspace = true web-time.workspace = true wgpu.workspace = true diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index 64976999388c..be1960aa653a 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -150,6 +150,7 @@ const MAX_ZOOM_FACTOR: f32 = 5.0; struct PendingFilePromise { recommended_application_id: Option, recommended_recording_id: Option, + force_store_info: bool, promise: poll_promise::Promise>, } @@ -573,17 +574,33 @@ impl App { store_context: Option<&StoreContext<'_>>, cmd: UICommand, ) { + let mut force_store_info = false; let active_application_id = store_context .and_then(|ctx| { ctx.hub .active_app() // Don't redirect data to the welcome screen. .filter(|&app_id| app_id != &StoreHub::welcome_screen_app_id()) + .cloned() }) - .cloned(); + // If we don't have any application ID to recommend (which means we are on the welcome screen), + // then just generate a new one using a UUID. + .or_else(|| Some(uuid::Uuid::new_v4().to_string().into())); let active_recording_id = store_context - .and_then(|ctx| ctx.hub.active_recording_id()) - .cloned(); + .and_then(|ctx| ctx.hub.active_recording_id().cloned()) + .or_else(|| { + // When we're on the welcome screen, there is no recording ID to recommend. + // But we want one, otherwise multiple things being dropped simultaneously on the + // welcome screen would end up in different recordings! + + // We're creating a recording just-in-time, directly from the viewer. + // We need those store infos or the data will just be silently ignored. + force_store_info = true; + + // NOTE: We don't override blueprints' store IDs anyhow, so it is sound to assume that + // this can only be a recording. + Some(re_log_types::StoreId::random(StoreKind::Recording)) + }); match cmd { UICommand::SaveRecording => { @@ -615,6 +632,7 @@ impl App { FileSource::FileDialog { recommended_application_id: None, recommended_recording_id: None, + force_store_info, }, file_path, ))); @@ -624,9 +642,6 @@ impl App { UICommand::Open => { let egui_ctx = egui_ctx.clone(); - // Open: we want to try and load into a new dedicated recording. - let recommended_application_id = None; - let recommended_recording_id = None; let promise = poll_promise::Promise::spawn_local(async move { let file = async_open_rrd_dialog().await; egui_ctx.request_repaint(); // Wake ui thread @@ -634,8 +649,9 @@ impl App { }); self.open_files_promise = Some(PendingFilePromise { - recommended_application_id, - recommended_recording_id, + recommended_application_id: None, + recommended_recording_id: None, + force_store_info, promise, }); } @@ -648,6 +664,7 @@ impl App { FileSource::FileDialog { recommended_application_id: active_application_id.clone(), recommended_recording_id: active_recording_id.clone(), + force_store_info, }, file_path, ))); @@ -657,10 +674,6 @@ impl App { UICommand::Import => { let egui_ctx = egui_ctx.clone(); - // Import: we want to try and load into the current recording. - let recommended_application_id = active_application_id; - let recommended_recording_id = active_recording_id; - let promise = poll_promise::Promise::spawn_local(async move { let file = async_open_rrd_dialog().await; egui_ctx.request_repaint(); // Wake ui thread @@ -668,8 +681,9 @@ impl App { }); self.open_files_promise = Some(PendingFilePromise { - recommended_application_id, - recommended_recording_id, + recommended_application_id: active_application_id.clone(), + recommended_recording_id: active_recording_id.clone(), + force_store_info, promise, }); } @@ -1364,17 +1378,33 @@ impl App { return; } + let mut force_store_info = false; let active_application_id = store_ctx .and_then(|ctx| { ctx.hub .active_app() // Don't redirect data to the welcome screen. .filter(|&app_id| app_id != &StoreHub::welcome_screen_app_id()) + .cloned() }) - .cloned(); + // If we don't have any application ID to recommend (which means we are on the welcome screen), + // then just generate a new one using a UUID. + .or_else(|| Some(uuid::Uuid::new_v4().to_string().into())); let active_recording_id = store_ctx - .and_then(|ctx| ctx.hub.active_recording_id()) - .cloned(); + .and_then(|ctx| ctx.hub.active_recording_id().cloned()) + .or_else(|| { + // When we're on the welcome screen, there is no recording ID to recommend. + // But we want one, otherwise multiple things being dropped simultaneously on the + // welcome screen would end up in different recordings! + + // We're creating a recording just-in-time, directly from the viewer. + // We need those store infos or the data will just be silently ignored. + force_store_info = true; + + // NOTE: We don't override blueprints' store IDs anyhow, so it is sound to assume that + // this can only be a recording. + Some(re_log_types::StoreId::random(StoreKind::Recording)) + }); for file in dropped_files { if let Some(bytes) = file.bytes { @@ -1384,6 +1414,7 @@ impl App { FileSource::DragAndDrop { recommended_application_id: active_application_id.clone(), recommended_recording_id: active_recording_id.clone(), + force_store_info, }, FileContents { name: file.name.clone(), @@ -1400,6 +1431,7 @@ impl App { FileSource::DragAndDrop { recommended_application_id: active_application_id.clone(), recommended_recording_id: active_recording_id.clone(), + force_store_info, }, path, ))); @@ -1650,6 +1682,7 @@ impl eframe::App for App { if let Some(PendingFilePromise { recommended_application_id, recommended_recording_id, + force_store_info, promise, }) = &self.open_files_promise { @@ -1660,6 +1693,7 @@ impl eframe::App for App { FileSource::FileDialog { recommended_application_id: recommended_application_id.clone(), recommended_recording_id: recommended_recording_id.clone(), + force_store_info: *force_store_info, }, file.clone(), ))); diff --git a/examples/rust/custom_data_loader/src/main.rs b/examples/rust/custom_data_loader/src/main.rs index 0811feab4203..bde1dd382484 100644 --- a/examples/rust/custom_data_loader/src/main.rs +++ b/examples/rust/custom_data_loader/src/main.rs @@ -9,7 +9,7 @@ use rerun::{ external::{anyhow, re_build_info, re_data_loader, re_log}, log::{Chunk, RowId}, - EntityPath, LoadedData, TimePoint, + DataLoader as _, EntityPath, LoadedData, TimePoint, }; fn main() -> anyhow::Result { @@ -81,7 +81,7 @@ fn hash_and_log( .opened_store_id .clone() .unwrap_or_else(|| settings.store_id.clone()); - let data = LoadedData::Chunk(store_id, chunk); + let data = LoadedData::Chunk(HashLoader::name(&HashLoader), store_id, chunk); tx.send(data).ok(); Ok(())