Skip to content

Commit

Permalink
Introduce the concepts of "default" and "active" blueprint. Make rese…
Browse files Browse the repository at this point in the history
…t return to default. (rerun-io#5678)

### What
 - Partial implementation of:
   - rerun-io#5583
   - rerun-io#5298
- This does not yet fully track a blueprint-per-recording-id, but
playing with this I think doing so might still be worth exploring.
 - Introduces a new "default blueprint" per app-id in the store-hub.
- Changes the ActivateStore -> BlueprintReady with a mechanism to
specify `make_default` and `make_active`
 - Introduces new python API: `send_blueprint`

Important aspect of implementation:
- When you click the reset button we now clone the default blueprint
into a new blueprint. This way we don't modify the blueprint that is the
target for the reset.

Future work we need to build on top of this (Feel free to push these
improvements here, or we can push to a future PR).
- [x] Biggest issue. This currently breaks File->Open / Drag-and-drop
since those now **just** set the default.
- Proposal: we should go ahead add the "force activate" bool into the
`ActivateStore` message. We should set this bool in the message when we
inject it into "File->Save" pathway. That way if you manually save a
blueprint, it will always load when you open it. But if you are opening
an RRD file that happens to have a blueprint it will still just use the
"default" behavior.
- [x] Should introduce SystemCommand & UI to clear the default blueprint
so resetting goes back to heuristics.

Future work:
- Should introduce SystemCommand & UI to set a different blueprint as
the default.
   - rerun-io#5691
- Notification to user when default blueprint changes. Probably better
as follow-on PR
   - rerun-io#5692
- **Make state observable** by supporting a selection of the AppId.
Probably better as follow-on PR:
   - rerun-io#5672

### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5678/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5678/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5678/index.html?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)!

- [PR Build Summary](https://build.rerun.io/pr/5678)
- [Docs
preview](https://rerun.io/preview/78935399ef8b4289c5ba6535f01e988e0cb26e70/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/78935399ef8b4289c5ba6535f01e988e0cb26e70/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
jleibs authored Mar 27, 2024
1 parent bd2f64a commit 4636188
Show file tree
Hide file tree
Showing 49 changed files with 643 additions and 213 deletions.
6 changes: 6 additions & 0 deletions crates/re_data_ui/src/entity_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ impl crate::DataUi for EntityDb {
}
}

if ctx.store_context.is_default_blueprint(self.store_id()) {
ui.add_space(8.0);
ui.label("This is the default blueprint")
.on_hover_text("When you reset the blueprint for the app, this blueprint will be used as the template.");
};

if verbosity == UiVerbosity::Full {
sibling_stores_ui(ctx, ui, self);
}
Expand Down
14 changes: 11 additions & 3 deletions crates/re_data_ui/src/item_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,17 @@ pub fn entity_db_button_ui(
}

if response.clicked() {
// Open the recording / switch to this blueprint…
ctx.command_sender
.send_system(SystemCommand::ActivateStore(store_id.clone()));
// When we click on a recording, we directly activate it. This is safe to do because
// it's non-destructive and recordings are immutable. Switching back is easy.
// We don't do the same thing for blueprints as swapping them can be much more disruptive.
// It is much less obvious how to undo a blueprint switch and what happened to your original
// blueprint.
// TODO(jleibs): We should still have an `Activate this Blueprint` button in the selection panel
// for the blueprint.
if store_id.kind == re_log_types::StoreKind::Recording {
ctx.command_sender
.send_system(SystemCommand::ActivateRecording(store_id.clone()));
}

// …and select the store in the selection panel.
// Note that we must do it in this order, since the selection state is stored in the recording.
Expand Down
14 changes: 11 additions & 3 deletions crates/re_data_ui/src/log_msg.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use re_log_types::{ArrowMsg, DataTable, LogMsg, SetStoreInfo, StoreInfo};
use re_log_types::{
ArrowMsg, BlueprintActivationCommand, DataTable, LogMsg, SetStoreInfo, StoreInfo,
};
use re_viewer_context::{UiVerbosity, ViewerContext};

use super::DataUi;
Expand All @@ -16,8 +18,14 @@ impl DataUi for LogMsg {
match self {
LogMsg::SetStoreInfo(msg) => msg.data_ui(ctx, ui, verbosity, query, store),
LogMsg::ArrowMsg(_, msg) => msg.data_ui(ctx, ui, verbosity, query, store),
LogMsg::ActivateStore(store_id) => {
ui.label(format!("ActivateStore({store_id})"));
LogMsg::BlueprintActivationCommand(BlueprintActivationCommand {
blueprint_id,
make_active,
make_default,
}) => {
ui.label(format!(
"BlueprintActivationCommand({blueprint_id}, make_active: {make_active}, make_default: {make_default})"
));
}
}
}
Expand Down
46 changes: 41 additions & 5 deletions crates/re_entity_db/src/entity_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl EntityDb {
self.add_data_table(table)?;
}

LogMsg::ActivateStore(_) => {
LogMsg::BlueprintActivationCommand(_) => {
// Not for us to handle
}
}
Expand Down Expand Up @@ -540,18 +540,54 @@ impl EntityDb {
.map(|msg| LogMsg::ArrowMsg(self.store_id().clone(), msg))
});

// Signal that the store is done loading.
// Important for blueprints.
let activate_store_msg = LogMsg::ActivateStore(self.store_id().clone());
// If this is a blueprint, make sure to include the `BlueprintActivationCommand` message.
// We generally use `to_messages` to export a blueprint via "save". In that
// case, we want to make the blueprint active and default when it's reloaded.
// TODO(jleibs): Coupling this with the stored file instead of injecting seems
// architecturally weird. Would be great if we didn't need this in `.rbl` files
// at all.
let blueprint_ready = if self.store_kind() == StoreKind::Blueprint {
let activate_cmd =
re_log_types::BlueprintActivationCommand::make_active(self.store_id().clone());

itertools::Either::Left(std::iter::once(Ok(activate_cmd.into())))
} else {
itertools::Either::Right(std::iter::empty())
};

let messages: Result<Vec<_>, _> = set_store_info_msg
.into_iter()
.chain(data_messages)
.chain(std::iter::once(Ok(activate_store_msg)))
.chain(blueprint_ready)
.collect();

messages
}

/// Make a clone of this [`EntityDb`] with a different [`StoreId`].
pub fn clone_with_new_id(&self, new_id: StoreId) -> Result<EntityDb, Error> {
self.store().sort_indices_if_needed();

let mut new_db = EntityDb::new(new_id.clone());

if let Some(store_info) = self.store_info() {
let mut new_info = store_info.clone();
new_info.store_id = new_id;

new_db.set_store_info(SetStoreInfo {
row_id: RowId::new(),
info: new_info,
});
}

new_db.data_source = self.data_source.clone();

for row in self.store().to_rows()? {
new_db.add_data_row(row)?;
}

Ok(new_db)
}
}

impl re_types_core::SizeBytes for EntityDb {
Expand Down
69 changes: 65 additions & 4 deletions crates/re_log_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,58 @@ impl std::fmt::Display for ApplicationId {

// ----------------------------------------------------------------------------

/// Command used for activating a blueprint once it has been fully transmitted.
///
/// This command serves two purposes:
/// - It is important that a blueprint is never activated before it has been fully
/// transmitted. Displaying, or allowing a user to modify, a half-transmitted
/// blueprint can cause confusion and bad interactions with the space view heuristics.
/// - Additionally, this command allows fine-tuning the activation behavior itself
/// by specifying whether the blueprint should be immediately activated, or only
/// become the default for future activations.
#[derive(Clone, Debug, PartialEq, Eq)] // `PartialEq` used for tests in another crate
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct BlueprintActivationCommand {
/// The blueprint this command refers to.
pub blueprint_id: StoreId,

/// Immediately make this the active blueprint for the associated `app_id`.
///
/// Note that setting this to `false` does not mean the blueprint may not still end
/// up becoming active. In particular, if `make_default` is true and there is no other
/// currently active blueprint.
pub make_active: bool,

/// Make this the default blueprint for the `app_id`.
///
/// The default blueprint will be used as the template when the user resets the
/// blueprint for the app. It will also become the active blueprint if no other
/// blueprint is currently active.
pub make_default: bool,
}

impl BlueprintActivationCommand {
/// Make `blueprint_id` the default blueprint for its associated `app_id`.
pub fn make_default(blueprint_id: StoreId) -> Self {
Self {
blueprint_id,
make_active: false,
make_default: true,
}
}

/// Immediately make `blueprint_id` the active blueprint for its associated `app_id`.
///
/// This also sets `make_default` to true.
pub fn make_active(blueprint_id: StoreId) -> Self {
Self {
blueprint_id,
make_active: true,
make_default: true,
}
}
}

/// The most general log message sent from the SDK to the server.
#[must_use]
#[derive(Clone, Debug, PartialEq)] // `PartialEq` used for tests in another crate
Expand All @@ -224,14 +276,15 @@ pub enum LogMsg {
/// This is so that the viewer can wait with activating the blueprint until it is
/// fully transmitted. Showing a half-transmitted blueprint can cause confusion,
/// and also lead to problems with space-view heuristics.
ActivateStore(StoreId),
BlueprintActivationCommand(BlueprintActivationCommand),
}

impl LogMsg {
pub fn store_id(&self) -> &StoreId {
match self {
Self::SetStoreInfo(msg) => &msg.info.store_id,
Self::ArrowMsg(store_id, _) | Self::ActivateStore(store_id) => store_id,
Self::ArrowMsg(store_id, _) => store_id,
Self::BlueprintActivationCommand(cmd) => &cmd.blueprint_id,
}
}

Expand All @@ -240,14 +293,22 @@ impl LogMsg {
LogMsg::SetStoreInfo(store_info) => {
store_info.info.store_id = new_store_id;
}
LogMsg::ArrowMsg(msg_store_id, _) | LogMsg::ActivateStore(msg_store_id) => {
*msg_store_id = new_store_id;
LogMsg::ArrowMsg(store_id, _) => {
*store_id = new_store_id;
}
LogMsg::BlueprintActivationCommand(cmd) => {
cmd.blueprint_id = new_store_id;
}
}
}
}

impl_into_enum!(SetStoreInfo, LogMsg, SetStoreInfo);
impl_into_enum!(
BlueprintActivationCommand,
LogMsg,
BlueprintActivationCommand
);

// ----------------------------------------------------------------------------

Expand Down
3 changes: 2 additions & 1 deletion crates/re_sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ mod spawn;
pub use spawn::{spawn, SpawnError, SpawnOptions};

pub use self::recording_stream::{
RecordingStream, RecordingStreamBuilder, RecordingStreamError, RecordingStreamResult,
forced_sink_path, RecordingStream, RecordingStreamBuilder, RecordingStreamError,
RecordingStreamResult,
};

pub use re_sdk_comms::{default_flush_timeout, default_server_addr};
Expand Down
39 changes: 38 additions & 1 deletion crates/re_sdk/src/log_sink.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::fmt;
use std::sync::Arc;

use parking_lot::RwLock;
use re_log_types::LogMsg;
use re_log_types::{BlueprintActivationCommand, LogMsg, StoreId};

/// Where the SDK sends its log messages.
pub trait LogSink: Send + Sync + 'static {
Expand Down Expand Up @@ -34,6 +34,35 @@ pub trait LogSink: Send + Sync + 'static {
/// flush it for any reason (e.g. a broken TCP connection for a [`TcpSink`]).
#[inline]
fn drop_if_disconnected(&self) {}

/// Send a blueprint directly to the log-sink.
///
/// This mirrors the behavior of [`crate::RecordingStream::send_blueprint`].
fn send_blueprint(&self, blueprint: Vec<LogMsg>, activation_cmd: BlueprintActivationCommand) {
let mut blueprint_id = None;
for msg in blueprint {
if blueprint_id.is_none() {
blueprint_id = Some(msg.store_id().clone());
}
self.send(msg);
}

if let Some(blueprint_id) = blueprint_id {
if blueprint_id == activation_cmd.blueprint_id {
// Let the viewer know that the blueprint has been fully received,
// and that it can now be activated.
// We don't want to activate half-loaded blueprints, because that can be confusing,
// and can also lead to problems with space-view heuristics.
self.send(activation_cmd.into());
} else {
re_log::warn!(
"Blueprint ID mismatch when sending blueprint: {} != {}. Ignoring activation.",
blueprint_id,
activation_cmd.blueprint_id
);
}
}
}
}

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -206,6 +235,14 @@ impl MemorySinkStorage {

Ok(buffer.into_inner())
}

#[inline]
/// Get the [`StoreId`] from the associated `RecordingStream` if it exists.
pub fn store_id(&self) -> Option<StoreId> {
self.rec
.as_ref()
.and_then(|rec| rec.store_info().map(|info| info.store_id.clone()))
}
}
// ----------------------------------------------------------------------------

Expand Down
Loading

0 comments on commit 4636188

Please sign in to comment.