Skip to content

Commit 91e981a

Browse files
teh-cmcteh-cmc
authored
Implement support for in-place drag-n-drop (#7880)
It is pretty tricky to get right (unsurprisingly) -- `StoreId`s and `RecordingId`s and `SetStoreInfo`s are all pretty finicky constructs and a lot can go wrong when loading arbitrarily things in place. But it works pretty nicely overall! https://github.com/user-attachments/assets/43f4e647-317a-47b5-a570-bee70716a2de * Fixes #5350
1 parent 853f1b1 commit 91e981a

File tree

10 files changed

+207
-114
lines changed

10 files changed

+207
-114
lines changed

‎crates/store/re_data_loader/src/lib.rs

Lines changed: 5 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,6 @@ pub struct DataLoaderSettings {
5353
pub application_id: Option<re_log_types::ApplicationId>,
5454

5555
/// The [`re_log_types::ApplicationId`] that is currently opened in the viewer, if any.
56-
//
57-
// TODO(#5350): actually support this
5856
pub opened_application_id: Option<re_log_types::ApplicationId>,
5957

6058
/// The recommended [`re_log_types::StoreId`] to log the data to, based on the surrounding context.
@@ -64,8 +62,6 @@ pub struct DataLoaderSettings {
6462
pub store_id: re_log_types::StoreId,
6563

6664
/// The [`re_log_types::StoreId`] that is currently opened in the viewer, if any.
67-
//
68-
// TODO(#5350): actually support this
6965
pub opened_store_id: Option<re_log_types::StoreId>,
7066

7167
/// What should the logged entity paths be prefixed with?
@@ -318,39 +314,18 @@ impl DataLoaderError {
318314
/// most convenient for them, whether it is raw components, arrow chunks or even
319315
/// full-on [`LogMsg`]s.
320316
pub enum LoadedData {
321-
Chunk(Chunk),
322-
ArrowMsg(ArrowMsg),
317+
Chunk(re_log_types::StoreId, Chunk),
318+
ArrowMsg(re_log_types::StoreId, ArrowMsg),
323319
LogMsg(LogMsg),
324320
}
325321

326-
impl From<Chunk> for LoadedData {
327-
#[inline]
328-
fn from(value: Chunk) -> Self {
329-
Self::Chunk(value)
330-
}
331-
}
332-
333-
impl From<ArrowMsg> for LoadedData {
334-
#[inline]
335-
fn from(value: ArrowMsg) -> Self {
336-
Self::ArrowMsg(value)
337-
}
338-
}
339-
340-
impl From<LogMsg> for LoadedData {
341-
#[inline]
342-
fn from(value: LogMsg) -> Self {
343-
Self::LogMsg(value)
344-
}
345-
}
346-
347322
impl LoadedData {
348323
/// Pack the data into a [`LogMsg`].
349-
pub fn into_log_msg(self, store_id: &re_log_types::StoreId) -> ChunkResult<LogMsg> {
324+
pub fn into_log_msg(self) -> ChunkResult<LogMsg> {
350325
match self {
351-
Self::Chunk(chunk) => Ok(LogMsg::ArrowMsg(store_id.clone(), chunk.to_arrow_msg()?)),
326+
Self::Chunk(store_id, chunk) => Ok(LogMsg::ArrowMsg(store_id, chunk.to_arrow_msg()?)),
352327

353-
Self::ArrowMsg(msg) => Ok(LogMsg::ArrowMsg(store_id.clone(), msg)),
328+
Self::ArrowMsg(store_id, msg) => Ok(LogMsg::ArrowMsg(store_id, msg)),
354329

355330
Self::LogMsg(msg) => Ok(msg),
356331
}

‎crates/store/re_data_loader/src/load_file.rs

Lines changed: 52 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use std::borrow::Cow;
22

3+
use ahash::{HashMap, HashMapExt};
34
use re_log_types::{FileSource, LogMsg};
45
use re_smart_channel::Sender;
56

6-
use crate::{extension, DataLoaderError, LoadedData};
7+
use crate::{DataLoaderError, LoadedData};
78

89
// ---
910

@@ -36,16 +37,7 @@ pub fn load_from_path(
3637

3738
let rx = load(settings, path, None)?;
3839

39-
// TODO(cmc): should we always unconditionally set store info though?
40-
// If we reach this point, then at least one compatible `DataLoader` has been found.
41-
let store_info = prepare_store_info(&settings.store_id, file_source, path);
42-
if let Some(store_info) = store_info {
43-
if tx.send(store_info).is_err() {
44-
return Ok(()); // other end has hung up.
45-
}
46-
}
47-
48-
send(&settings.store_id, rx, tx);
40+
send(settings.clone(), file_source, path.to_owned(), rx, tx);
4941

5042
Ok(())
5143
}
@@ -72,16 +64,7 @@ pub fn load_from_file_contents(
7264

7365
let data = load(settings, filepath, Some(contents))?;
7466

75-
// TODO(cmc): should we always unconditionally set store info though?
76-
// If we reach this point, then at least one compatible `DataLoader` has been found.
77-
let store_info = prepare_store_info(&settings.store_id, file_source, filepath);
78-
if let Some(store_info) = store_info {
79-
if tx.send(store_info).is_err() {
80-
return Ok(()); // other end has hung up.
81-
}
82-
}
83-
84-
send(&settings.store_id, data, tx);
67+
send(settings.clone(), file_source, filepath.to_owned(), data, tx);
8568

8669
Ok(())
8770
}
@@ -93,35 +76,25 @@ pub(crate) fn prepare_store_info(
9376
store_id: &re_log_types::StoreId,
9477
file_source: FileSource,
9578
path: &std::path::Path,
96-
) -> Option<LogMsg> {
79+
) -> LogMsg {
9780
re_tracing::profile_function!(path.display().to_string());
9881

9982
use re_log_types::SetStoreInfo;
10083

10184
let app_id = re_log_types::ApplicationId(path.display().to_string());
10285
let store_source = re_log_types::StoreSource::File { file_source };
10386

104-
let ext = extension(path);
105-
let is_rrd = crate::SUPPORTED_RERUN_EXTENSIONS.contains(&ext.as_str());
106-
107-
(!is_rrd).then(|| {
108-
LogMsg::SetStoreInfo(SetStoreInfo {
109-
row_id: *re_chunk::RowId::new(),
110-
info: re_log_types::StoreInfo {
111-
application_id: app_id.clone(),
112-
store_id: store_id.clone(),
113-
cloned_from: None,
114-
is_official_example: false,
115-
started: re_log_types::Time::now(),
116-
store_source,
117-
// NOTE: If this is a natively supported file, it will go through one of the
118-
// builtin dataloaders, i.e. the local version.
119-
// Otherwise, it will go through an arbitrary external loader, at which point we
120-
// have no certainty what the version is.
121-
store_version: crate::is_supported_file_extension(ext.as_str())
122-
.then_some(re_build_info::CrateVersion::LOCAL),
123-
},
124-
})
87+
LogMsg::SetStoreInfo(SetStoreInfo {
88+
row_id: *re_chunk::RowId::new(),
89+
info: re_log_types::StoreInfo {
90+
application_id: app_id.clone(),
91+
store_id: store_id.clone(),
92+
cloned_from: None,
93+
is_official_example: false,
94+
started: re_log_types::Time::now(),
95+
store_source,
96+
store_version: Some(re_build_info::CrateVersion::LOCAL),
97+
},
12598
})
12699
}
127100

@@ -288,32 +261,63 @@ pub(crate) fn load(
288261
///
289262
/// Runs asynchronously from another thread on native, synchronously on wasm.
290263
pub(crate) fn send(
291-
store_id: &re_log_types::StoreId,
264+
settings: crate::DataLoaderSettings,
265+
file_source: FileSource,
266+
path: std::path::PathBuf,
292267
rx_loader: std::sync::mpsc::Receiver<LoadedData>,
293268
tx: &Sender<LogMsg>,
294269
) {
295270
spawn({
296271
re_tracing::profile_function!();
297272

273+
let mut store_info_tracker: HashMap<re_log_types::StoreId, bool> = HashMap::new();
274+
298275
let tx = tx.clone();
299-
let store_id = store_id.clone();
300276
move || {
301277
// ## Ignoring channel errors
302278
//
303279
// Not our problem whether or not the other end has hung up, but we still want to
304280
// poll the channel in any case so as to make sure that the data producer
305281
// doesn't get stuck.
306282
for data in rx_loader {
307-
let msg = match data.into_log_msg(&store_id) {
308-
Ok(msg) => msg,
283+
let msg = match data.into_log_msg() {
284+
Ok(msg) => {
285+
let store_info = match &msg {
286+
LogMsg::SetStoreInfo(set_store_info) => {
287+
Some((set_store_info.info.store_id.clone(), true))
288+
}
289+
LogMsg::ArrowMsg(store_id, _arrow_msg) => {
290+
Some((store_id.clone(), false))
291+
}
292+
LogMsg::BlueprintActivationCommand(_) => None,
293+
};
294+
295+
if let Some((store_id, store_info_created)) = store_info {
296+
*store_info_tracker.entry(store_id).or_default() |= store_info_created;
297+
}
298+
299+
msg
300+
}
309301
Err(err) => {
310-
re_log::error!(%err, %store_id, "Couldn't serialize component data");
302+
re_log::error!(%err, "Couldn't serialize component data");
311303
continue;
312304
}
313305
};
314306
tx.send(msg).ok();
315307
}
316308

309+
for (store_id, store_info_already_created) in store_info_tracker {
310+
let is_a_preexisting_recording =
311+
Some(&store_id) == settings.opened_store_id.as_ref();
312+
313+
if store_info_already_created || is_a_preexisting_recording {
314+
continue;
315+
}
316+
317+
let store_info = prepare_store_info(&store_id, file_source.clone(), &path);
318+
tx.send(store_info).ok();
319+
}
320+
317321
tx.quit(None).ok();
318322
}
319323
});

‎crates/store/re_data_loader/src/loader_archetype.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ impl DataLoader for ArchetypeLoader {
5050

5151
fn load_from_file_contents(
5252
&self,
53-
_settings: &crate::DataLoaderSettings,
53+
settings: &crate::DataLoaderSettings,
5454
filepath: std::path::PathBuf,
5555
contents: std::borrow::Cow<'_, [u8]>,
5656
tx: std::sync::mpsc::Sender<LoadedData>,
@@ -133,8 +133,13 @@ impl DataLoader for ArchetypeLoader {
133133
)?);
134134
}
135135

136+
let store_id = settings
137+
.opened_store_id
138+
.clone()
139+
.unwrap_or_else(|| settings.store_id.clone());
136140
for row in rows {
137-
if tx.send(row.into()).is_err() {
141+
let data = LoadedData::Chunk(store_id.clone(), row);
142+
if tx.send(data).is_err() {
138143
break; // The other end has decided to hang up, not our problem.
139144
}
140145
}

‎crates/store/re_data_loader/src/loader_external.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ use std::{
66
use ahash::HashMap;
77
use once_cell::sync::Lazy;
88

9+
use crate::LoadedData;
10+
911
// ---
1012

1113
/// To register a new external data loader, simply add an executable in your $PATH whose name
@@ -318,7 +320,9 @@ fn decode_and_stream<R: std::io::Read>(
318320
continue;
319321
}
320322
};
321-
if tx.send(msg.into()).is_err() {
323+
324+
let data = LoadedData::LogMsg(msg);
325+
if tx.send(data).is_err() {
322326
break; // The other end has decided to hang up, not our problem.
323327
}
324328
}

‎crates/store/re_data_loader/src/loader_rrd.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ use re_log_encoding::decoder::Decoder;
44
use crossbeam::channel::Receiver;
55
use re_log_types::{ApplicationId, StoreId};
66

7+
use crate::LoadedData;
8+
79
// ---
810

911
/// Loads data from any `rrd` file or in-memory contents.
@@ -193,7 +195,8 @@ fn decode_and_stream<R: std::io::Read>(
193195
msg
194196
};
195197

196-
if tx.send(msg.into()).is_err() {
198+
let data = LoadedData::LogMsg(msg);
199+
if tx.send(data).is_err() {
197200
break; // The other end has decided to hang up, not our problem.
198201
}
199202
}

‎crates/store/re_data_source/src/data_source.rs

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,11 @@ impl DataSource {
164164
// or not.
165165
let shared_store_id =
166166
re_log_types::StoreId::random(re_log_types::StoreKind::Recording);
167-
let settings = re_data_loader::DataLoaderSettings::recommended(shared_store_id);
167+
let settings = re_data_loader::DataLoaderSettings {
168+
opened_application_id: file_source.recommended_application_id().cloned(),
169+
opened_store_id: file_source.recommended_recording_id().cloned(),
170+
..re_data_loader::DataLoaderSettings::recommended(shared_store_id)
171+
};
168172
re_data_loader::load_from_path(&settings, file_source, &path, &tx)
169173
.with_context(|| format!("{path:?}"))?;
170174

@@ -188,7 +192,11 @@ impl DataSource {
188192
// or not.
189193
let shared_store_id =
190194
re_log_types::StoreId::random(re_log_types::StoreKind::Recording);
191-
let settings = re_data_loader::DataLoaderSettings::recommended(shared_store_id);
195+
let settings = re_data_loader::DataLoaderSettings {
196+
opened_application_id: file_source.recommended_application_id().cloned(),
197+
opened_store_id: file_source.recommended_recording_id().cloned(),
198+
..re_data_loader::DataLoaderSettings::recommended(shared_store_id)
199+
};
192200
re_data_loader::load_from_file_contents(
193201
&settings,
194202
file_source,
@@ -248,12 +256,15 @@ fn test_data_source_from_uri() {
248256
];
249257
let ws = ["ws://foo.zip", "wss://foo.zip", "127.0.0.1"];
250258

251-
let file_source = FileSource::DragAndDrop;
259+
let file_source = FileSource::DragAndDrop {
260+
recommended_application_id: None,
261+
recommended_recording_id: None,
262+
};
252263

253264
for uri in file {
254265
assert!(
255266
matches!(
256-
DataSource::from_uri(file_source, uri.to_owned()),
267+
DataSource::from_uri(file_source.clone(), uri.to_owned()),
257268
DataSource::FilePath { .. }
258269
),
259270
"Expected {uri:?} to be categorized as FilePath"
@@ -263,7 +274,7 @@ fn test_data_source_from_uri() {
263274
for uri in http {
264275
assert!(
265276
matches!(
266-
DataSource::from_uri(file_source, uri.to_owned()),
277+
DataSource::from_uri(file_source.clone(), uri.to_owned()),
267278
DataSource::RrdHttpUrl { .. }
268279
),
269280
"Expected {uri:?} to be categorized as RrdHttpUrl"
@@ -273,7 +284,7 @@ fn test_data_source_from_uri() {
273284
for uri in ws {
274285
assert!(
275286
matches!(
276-
DataSource::from_uri(file_source, uri.to_owned()),
287+
DataSource::from_uri(file_source.clone(), uri.to_owned()),
277288
DataSource::WebSocketAddr(_)
278289
),
279290
"Expected {uri:?} to be categorized as WebSocketAddr"

0 commit comments

Comments
 (0)