Skip to content

Commit

Permalink
implement consistent open/import behavior for all cases
Browse files Browse the repository at this point in the history
  • Loading branch information
teh-cmc committed Nov 5, 2024
1 parent 3886323 commit b614e00
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 26 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5376,6 +5376,7 @@ dependencies = [
"re_types",
"tempfile",
"thiserror",
"uuid",
"walkdir",
]

Expand Down Expand Up @@ -6415,6 +6416,7 @@ dependencies = [
"strum",
"strum_macros",
"thiserror",
"uuid",
"wasm-bindgen",
"wasm-bindgen-futures",
"web-sys",
Expand Down
1 change: 1 addition & 0 deletions crates/store/re_data_loader/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
8 changes: 8 additions & 0 deletions crates/store/re_data_loader/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<re_log_types::StoreId>,

/// 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<EntityPath>,

Expand All @@ -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(),
}
Expand All @@ -91,6 +98,7 @@ impl DataLoaderSettings {
opened_application_id,
store_id,
opened_store_id,
force_store_info: _,
entity_path_prefix,
timepoint,
} = self;
Expand Down
22 changes: 13 additions & 9 deletions crates/store/re_data_loader/src/load_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand All @@ -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(())
}
Expand All @@ -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,
Expand Down Expand Up @@ -263,7 +262,6 @@ pub(crate) fn load(
pub(crate) fn send(
settings: crate::DataLoaderSettings,
file_source: FileSource,
path: std::path::PathBuf,
rx_loader: std::sync::mpsc::Receiver<LoadedData>,
tx: &Sender<LogMsg>,
) {
Expand Down Expand Up @@ -310,11 +308,17 @@ pub(crate) fn send(
let is_a_preexisting_recording =
Some(&store_id) == settings.opened_store_id.as_ref();

if store_info_already_created || is_a_preexisting_recording {
if !settings.force_store_info
&& (store_info_already_created || is_a_preexisting_recording)
{
continue;
}

let store_info = prepare_store_info(&store_id, file_source.clone(), &path);
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();
}

Expand Down
3 changes: 3 additions & 0 deletions crates/store/re_data_source/src/data_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down
25 changes: 25 additions & 0 deletions crates/store/re_log_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,12 @@ pub enum FileSource {
/// The [`StoreId`] that the viewer heuristically recommends should be used when loading
/// this data source, based on the surrounding context.
recommended_recording_id: Option<StoreId>,

/// 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).
force_store_info: bool,
},

FileDialog {
Expand All @@ -428,6 +434,12 @@ pub enum FileSource {
/// The [`StoreId`] that the viewer heuristically recommends should be used when loading
/// this data source, based on the surrounding context.
recommended_recording_id: Option<StoreId>,

/// 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).
force_store_info: bool,
},

Sdk,
Expand Down Expand Up @@ -463,6 +475,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.
Expand Down
1 change: 1 addition & 0 deletions crates/top/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
1 change: 1 addition & 0 deletions crates/viewer/re_viewer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
68 changes: 51 additions & 17 deletions crates/viewer/re_viewer/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ const MAX_ZOOM_FACTOR: f32 = 5.0;
struct PendingFilePromise {
recommended_application_id: Option<ApplicationId>,
recommended_recording_id: Option<re_log_types::StoreId>,
force_store_info: bool,
promise: poll_promise::Promise<Vec<re_data_source::FileContents>>,
}

Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -615,6 +632,7 @@ impl App {
FileSource::FileDialog {
recommended_application_id: None,
recommended_recording_id: None,
force_store_info,
},
file_path,
)));
Expand All @@ -624,18 +642,16 @@ 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
file
});

self.open_files_promise = Some(PendingFilePromise {
recommended_application_id,
recommended_recording_id,
recommended_application_id: None,
recommended_recording_id: None,
force_store_info,
promise,
});
}
Expand All @@ -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,
)));
Expand All @@ -657,19 +674,16 @@ 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
file
});

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,
});
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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(),
Expand All @@ -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,
)));
Expand Down Expand Up @@ -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
{
Expand All @@ -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(),
)));
Expand Down

0 comments on commit b614e00

Please sign in to comment.