Skip to content

Commit

Permalink
Basic persistence for blueprints (#2578)
Browse files Browse the repository at this point in the history
### What
When moving over to the new blueprint store, we temporarily lost the
ability to persist blueprints.

This is one of the major regressions currently preventing us from
releasing main (See: #2529)

This PR adds new methods to the `store_hub`:
 - `persist_app_blueprints`
 - `try_to_load_persisted_blueprint`
 
The blueprints are stored using the standard rrd file format. They go
into a sub-directory of our app-state storage:
- Linux: /home/UserName/.local/share/rerun/blueprints/
- macOS: /Users/UserName/Library/Application Support/rerun/blueprints/
- Windows: C:\Users\UserName\AppData\Roaming\rerun\blueprints\

We only store a single blueprint in this folder per app-id. 

Any time we change the app_id via the `store_hub` (such as when loading
an rrd) we will search the directory for a persisted blueprint and then
load it. We track the `insert_id` on the active blueprint to determine
if we need to save back out to disk again. I've tested to confirm that
in most cases when we restore from blueprint we don't do anything weird
like re-modify the blueprint and save it again.

### Additional Notes:
The way that DataBlueprint/AutoValues work required a workaround that I
don't particularly love, but does seems to be workable until we refactor
to get rid of more of the serde business.
- After implementing blueprint-loading, I noticed that every time we
start the app fresh, we see a cycle where the AutoValue for
`pinhole_image_plane_distance` goes from Auto(100.06751) -> Auto(1) ->
Auto(100.03121). Even ignoring the transient jump due to view bounds
being a frame delayed to populate, the Auto-value itself is unstable
from run to run. This meant that every time we started the app, we would
do a new insertion which would cause the blueprint to immediately get
saved again.
- The work-around is to replace the usage of `!=` (PartialEq) with a
conceptually equivalent method `has_edits` for the DataBlueprintTree. We
now consider all values of Auto to be considered a match. This seems to
work out fine since we (should?) always recompute Auto values at the
beginning of each frame.
- Longer term, I think the right solution is to move the Auto values out
of the blueprint and into the AppState so they persist frame-to-frame
and don't even show up in the blueprint comparison logic. However,
DataBlueprintTree is a complicated piece of work, and plumbing through
that state is proving to be challenging.
   
### Future work
This currently only implements file-backed storage, so doesn't work with
web-view.
 - #2579

### Testing:
* Run colmap fiat:
```
python examples/python/structure_from_motion/main.py --dataset colmap_fiat
```
* Adjust views
* Exit application
* Check the md5sum of the blueprint:
```
$ md5sum /home/jleibs/.local/share/rerun/blueprints/structure_from_motion.blueprint 
03fab330d9c23ac3da1163c854aa8518  /home/jleibs/.local/share/rerun/blueprints/structure_from_motion.blueprint
```
* Run colmap fiat a *second* time.
* Confirm the layout persists:

![image](https://github.com/rerun-io/rerun/assets/3312232/036d6ae9-6d3e-4abf-a981-cc46a7b6fe71)
* Close rerun and verify the blueprint hasn't been changed:
```
$ md5sum /home/jleibs/.local/share/rerun/blueprints/structure_from_motion.blueprint 
03fab330d9c23ac3da1163c854aa8518  /home/jleibs/.local/share/rerun/blueprints/structure_from_motion.blueprint
```


### 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 [demo.rerun.io](https://demo.rerun.io/pr/2578) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/2578)
- [Docs
preview](https://rerun.io/preview/pr%3Ajleibs%2Fsave_blueprints/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Ajleibs%2Fsave_blueprints/examples)
  • Loading branch information
jleibs committed Jul 4, 2023
1 parent a2e6739 commit 52f51c1
Show file tree
Hide file tree
Showing 18 changed files with 411 additions and 90 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/labels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,4 @@ jobs:
with:
mode: minimum
count: 1
labels: "📊 analytics, 🪳 bug, C/C++ SDK, codegen/idl, 🧑‍💻 dev experience, dependencies, 📖 documentation, 💬 discussion, examples, 📉 performance, 🐍 python API, ⛃ re_datastore, 📺 re_viewer, 🔺 re_renderer, 🚜 refactor, ⛴ release, 🦀 rust SDK, 🔨 testing, ui, 🕸️ web"
labels: "📊 analytics, , 🟦 blueprint, 🪳 bug, C/C++ SDK, codegen/idl, 🧑‍💻 dev experience, dependencies, 📖 documentation, 💬 discussion, examples, 📉 performance, 🐍 python API, ⛃ re_datastore, 📺 re_viewer, 🔺 re_renderer, 🚜 refactor, ⛴ release, 🦀 rust SDK, 🔨 testing, ui, 🕸️ web"
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/re_arrow_store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub mod polars_util;
pub mod test_util;

pub use self::arrow_util::ArrayExt;
pub use self::store::{DataStore, DataStoreConfig};
pub use self::store::{DataStore, DataStoreConfig, StoreGeneration};
pub use self::store_gc::GarbageCollectionTarget;
pub use self::store_read::{LatestAtQuery, RangeQuery};
pub use self::store_stats::{DataStoreRowStats, DataStoreStats};
Expand Down
10 changes: 10 additions & 0 deletions crates/re_arrow_store/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ impl std::ops::DerefMut for ClusterCellCache {

// ---

/// Incremented on each edit
#[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct StoreGeneration(u64);

/// A complete data store: covers all timelines, all entities, everything.
///
/// ## Debugging
Expand Down Expand Up @@ -264,6 +268,12 @@ impl DataStore {
"rerun.insert_id".into()
}

/// Return the current `StoreGeneration`. This can be used to determine whether the
/// database has been modified since the last time it was queried.
pub fn generation(&self) -> StoreGeneration {
StoreGeneration(self.insert_id)
}

/// See [`Self::cluster_key`] for more information about the cluster key.
pub fn cluster_key(&self) -> ComponentName {
self.cluster_key
Expand Down
10 changes: 10 additions & 0 deletions crates/re_data_store/src/editable_auto_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,14 @@ where
self
}
}

/// Determine whether this `EditableAutoValue` has user-edits relative to another `EditableAutoValue`
/// If both values are `Auto`, then it is not considered edited.
pub fn has_edits(&self, other: &Self) -> bool {
match (self, other) {
(EditableAutoValue::UserEdited(s), EditableAutoValue::UserEdited(o)) => s != o,
(EditableAutoValue::Auto(_), EditableAutoValue::Auto(_)) => false,
_ => true,
}
}
}
34 changes: 34 additions & 0 deletions crates/re_data_store/src/entity_properties.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ impl EntityPropertyMap {
pub fn iter(&self) -> impl Iterator<Item = (&EntityPath, &EntityProperties)> {
self.props.iter()
}

/// Determine whether this `EntityPropertyMap` has user-edits relative to another `EntityPropertyMap`
pub fn has_edits(&self, other: &Self) -> bool {
self.props.len() != other.props.len()
|| self.props.iter().any(|(key, val)| {
other
.props
.get(key)
.map_or(true, |other_val| val.has_edits(other_val))
})
}
}

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -116,6 +127,29 @@ impl EntityProperties {
.clone(),
}
}

/// Determine whether this `EntityProperty` has user-edits relative to another `EntityProperty`
pub fn has_edits(&self, other: &Self) -> bool {
let Self {
visible,
visible_history,
interactive,
color_mapper,
pinhole_image_plane_distance,
backproject_depth,
depth_from_world_scale,
backproject_radius_scale,
} = self;

visible != &other.visible
|| visible_history != &other.visible_history
|| interactive != &other.interactive
|| color_mapper.has_edits(&other.color_mapper)
|| pinhole_image_plane_distance.has_edits(&other.pinhole_image_plane_distance)
|| backproject_depth.has_edits(&other.backproject_depth)
|| depth_from_world_scale.has_edits(&other.depth_from_world_scale)
|| backproject_radius_scale.has_edits(&other.backproject_radius_scale)
}
}

// ----------------------------------------------------------------------------
Expand Down
6 changes: 6 additions & 0 deletions crates/re_data_store/src/store_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,12 @@ impl StoreDb {
+ self.entity_db.data_store.num_temporal_rows() as usize
}

/// Return the current `StoreGeneration`. This can be used to determine whether the
/// database has been modified since the last time it was queried.
pub fn generation(&self) -> re_arrow_store::StoreGeneration {
self.entity_db.data_store.generation()
}

pub fn is_empty(&self) -> bool {
self.recording_msg.is_none() && self.num_rows() == 0
}
Expand Down
62 changes: 50 additions & 12 deletions crates/re_space_view/src/data_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use slotmap::SlotMap;
use smallvec::{smallvec, SmallVec};

/// A grouping of several data-blueprints.
#[derive(Clone, Default, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Default, serde::Deserialize, serde::Serialize)]
pub struct DataBlueprintGroup {
pub display_name: String,

Expand All @@ -31,9 +31,29 @@ pub struct DataBlueprintGroup {
pub entities: BTreeSet<EntityPath>,
}

impl DataBlueprintGroup {
/// Determine whether this `DataBlueprints` has user-edits relative to another `DataBlueprints`
fn has_edits(&self, other: &Self) -> bool {
let Self {
display_name,
properties_individual,
properties_projected: _,
parent,
children,
entities,
} = self;

display_name != &other.display_name
|| properties_individual.has_edits(&other.properties_individual)
|| parent != &other.parent
|| children != &other.children
|| entities != &other.entities
}
}

/// Data blueprints for all entity paths in a space view.
#[derive(Clone, Default, PartialEq, serde::Deserialize, serde::Serialize)]
struct DataBlueprints {
#[derive(Clone, Default, serde::Deserialize, serde::Serialize)]
pub struct DataBlueprints {
/// Individual settings. Mutate this.
individual: EntityPropertyMap,

Expand All @@ -44,6 +64,19 @@ struct DataBlueprints {
projected: EntityPropertyMap,
}

// Manually implement `PartialEq` since projected is serde skip
impl DataBlueprints {
/// Determine whether this `DataBlueprints` has user-edits relative to another `DataBlueprints`
fn has_edits(&self, other: &Self) -> bool {
let Self {
individual,
projected: _,
} = self;

individual.has_edits(&other.individual)
}
}

/// Tree of all data blueprint groups for a single space view.
#[derive(Clone, serde::Deserialize, serde::Serialize)]
pub struct DataBlueprintTree {
Expand Down Expand Up @@ -71,9 +104,9 @@ pub struct DataBlueprintTree {
data_blueprints: DataBlueprints,
}

// Manually implement PartialEq since slotmap doesn't
impl PartialEq for DataBlueprintTree {
fn eq(&self, other: &Self) -> bool {
/// Determine whether this `DataBlueprintTree` has user-edits relative to another `DataBlueprintTree`
impl DataBlueprintTree {
pub fn has_edits(&self, other: &Self) -> bool {
let Self {
groups,
path_to_group,
Expand All @@ -82,12 +115,17 @@ impl PartialEq for DataBlueprintTree {
data_blueprints,
} = self;

// Note: this could fail unexpectedly if slotmap iteration order is unstable.
groups.iter().zip(other.groups.iter()).all(|(x, y)| x == y)
&& *path_to_group == other.path_to_group
&& *entity_paths == other.entity_paths
&& *root_group_handle == other.root_group_handle
&& *data_blueprints == other.data_blueprints
groups.len() != other.groups.len()
|| groups.iter().any(|(key, val)| {
other
.groups
.get(key)
.map_or(true, |other_val| val.has_edits(other_val))
})
|| *path_to_group != other.path_to_group
|| *entity_paths != other.entity_paths
|| *root_group_handle != other.root_group_handle
|| data_blueprints.has_edits(&other.data_blueprints)
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/re_viewer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ arrow2.workspace = true
arrow2_convert.workspace = true
bytemuck.workspace = true
cfg-if.workspace = true
directories-next = "2.0.0"
eframe = { workspace = true, default-features = false, features = [
"default_fonts",
"persistence",
Expand Down
85 changes: 19 additions & 66 deletions crates/re_viewer/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ impl App {
}
#[cfg(not(target_arch = "wasm32"))]
SystemCommand::LoadRrd(path) => {
if let Some(rrd) = crate::loading::load_file_path(&path) {
let with_notification = true;
if let Some(rrd) = crate::loading::load_file_path(&path, with_notification) {
store_hub.add_bundle(rrd);
}
}
Expand Down Expand Up @@ -763,7 +764,8 @@ impl App {

#[cfg(not(target_arch = "wasm32"))]
if let Some(path) = &file.path {
if let Some(rrd) = crate::loading::load_file_path(path) {
let with_notification = true;
if let Some(rrd) = crate::loading::load_file_path(path, with_notification) {
self.on_rrd_loaded(store_hub, rrd);
}
}
Expand All @@ -778,7 +780,20 @@ impl eframe::App for App {

fn save(&mut self, storage: &mut dyn eframe::Storage) {
if self.startup_options.persist_state {
// Save the app state
eframe::set_value(storage, eframe::APP_KEY, &self.state);

// Save the blueprints
// TODO(2579): implement web-storage for blueprints as well
#[cfg(not(target_arch = "wasm32"))]
if let Some(hub) = &mut self.store_hub {
match hub.persist_app_blueprints() {
Ok(f) => f,
Err(err) => {
re_log::error!("Saving blueprints failed: {err}");
}
};
}
}
}

Expand Down Expand Up @@ -1032,6 +1047,8 @@ fn save(
store_context: Option<&StoreContext<'_>>,
loop_selection: Option<(re_data_store::Timeline, re_log_types::TimeRangeF)>,
) {
use crate::saving::save_database_to_file;

let Some(store_db) = store_context.as_ref().and_then(|view| view.recording) else {
// NOTE: Can only happen if saving through the command palette.
re_log::error!("No data to save!");
Expand Down Expand Up @@ -1062,67 +1079,3 @@ fn save(
}
}
}

/// Returns a closure that, when run, will save the contents of the current database
/// to disk, at the specified `path`.
///
/// If `time_selection` is specified, then only data for that specific timeline over that
/// specific time range will be accounted for.
#[cfg(not(target_arch = "wasm32"))]
fn save_database_to_file(
store_db: &StoreDb,
path: std::path::PathBuf,
time_selection: Option<(re_data_store::Timeline, re_log_types::TimeRangeF)>,
) -> anyhow::Result<impl FnOnce() -> anyhow::Result<std::path::PathBuf>> {
use itertools::Itertools as _;
use re_arrow_store::TimeRange;

re_tracing::profile_scope!("dump_messages");

let begin_rec_msg = store_db
.recording_msg()
.map(|msg| LogMsg::SetStoreInfo(msg.clone()));

let ent_op_msgs = store_db
.iter_entity_op_msgs()
.map(|msg| LogMsg::EntityPathOpMsg(store_db.store_id().clone(), msg.clone()))
.collect_vec();

let time_filter = time_selection.map(|(timeline, range)| {
(
timeline,
TimeRange::new(range.min.floor(), range.max.ceil()),
)
});
let data_msgs: Result<Vec<_>, _> = store_db
.entity_db
.data_store
.to_data_tables(time_filter)
.map(|table| {
table
.to_arrow_msg()
.map(|msg| LogMsg::ArrowMsg(store_db.store_id().clone(), msg))
})
.collect();

use anyhow::Context as _;
let data_msgs = data_msgs.with_context(|| "Failed to export to data tables")?;

let msgs = std::iter::once(begin_rec_msg)
.flatten() // option
.chain(ent_op_msgs)
.chain(data_msgs);

Ok(move || {
re_tracing::profile_scope!("save_to_file");

use anyhow::Context as _;
let file = std::fs::File::create(path.as_path())
.with_context(|| format!("Failed to create file at {path:?}"))?;

let encoding_options = re_log_encoding::EncodingOptions::COMPRESSED;
re_log_encoding::encoder::encode_owned(encoding_options, msgs, file)
.map(|_| path)
.context("Message encode")
})
}
1 change: 1 addition & 0 deletions crates/re_viewer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ mod loading;
#[cfg(not(target_arch = "wasm32"))]
mod profiler;
mod remote_viewer_app;
mod saving;
mod screenshotter;
mod store_hub;
mod ui;
Expand Down
10 changes: 7 additions & 3 deletions crates/re_viewer/src/loading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,23 @@ use crate::StoreBundle;

#[cfg(not(target_arch = "wasm32"))]
#[must_use]
pub fn load_file_path(path: &std::path::Path) -> Option<StoreBundle> {
pub fn load_file_path(path: &std::path::Path, with_notifications: bool) -> Option<StoreBundle> {
fn load_file_path_impl(path: &std::path::Path) -> anyhow::Result<StoreBundle> {
re_tracing::profile_function!();
use anyhow::Context as _;
let file = std::fs::File::open(path).context("Failed to open file")?;
StoreBundle::from_rrd(file)
}

re_log::info!("Loading {path:?}…");
if with_notifications {
re_log::info!("Loading {path:?}…");
}

match load_file_path_impl(path) {
Ok(mut rrd) => {
re_log::info!("Loaded {path:?}");
if with_notifications {
re_log::info!("Loaded {path:?}");
}
for store_db in rrd.store_dbs_mut() {
store_db.data_source = Some(re_smart_channel::SmartChannelSource::Files {
paths: vec![path.into()],
Expand Down

1 comment on commit 52f51c1

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.25.

Benchmark suite Current: 52f51c1 Previous: a2e6739 Ratio
batch_points_arrow/encode_log_msg 87530 ns/iter (± 118) 49459 ns/iter (± 135) 1.77

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.