Skip to content

Commit

Permalink
Make selection history global instead of per recordings (#5739)
Browse files Browse the repository at this point in the history
### What

The selection history is now global, so the back/forward button work as
expected when selecting various recordings.

Note (quoting my comment in the original issue):
> This is technically a functional regression. Before, we could have a
different selection state for each recording (e.g "/world/robot" is
selected in recording A and "/world/camera" is selected in recording B).
However, in practice that's no longer true since recording have been
made selectable: switching to a new recording now implies selecting said
recording, in turn clearing the selection. This issue would make it
harder to go back to the previous behaviour.

- Fixes #5736 


https://github.com/rerun-io/rerun/assets/49431240/d376605c-5306-4377-bcfa-acd9479c54d7


### 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/5739/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5739/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/5739/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/5739)
- [Docs
preview](https://rerun.io/preview/3c9cbb2c945b6af1ddba49b0838de6227f8a4a2c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/3c9cbb2c945b6af1ddba49b0838de6227f8a4a2c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Jeremy Leibs <[email protected]>
  • Loading branch information
abey79 and jleibs authored Apr 2, 2024
1 parent e66d38e commit a671f02
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 72 deletions.
19 changes: 2 additions & 17 deletions crates/re_data_ui/src/item_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,22 +695,7 @@ pub fn entity_db_button_ui(
.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.
// That's also why we use a command to set the selection.
match store_id.kind {
re_log_types::StoreKind::Recording => {
ctx.command_sender.send_system(SystemCommand::SetSelection {
recording_id: Some(store_id),
item,
});
}
re_log_types::StoreKind::Blueprint => {
ctx.command_sender.send_system(SystemCommand::SetSelection {
recording_id: None,
item,
});
}
}
ctx.command_sender
.send_system(SystemCommand::SetSelection(item));
}
}
9 changes: 9 additions & 0 deletions crates/re_log_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ impl StoreId {
}
}

#[inline]
pub fn empty_recording() -> Self {
Self::from_string(StoreKind::Recording, "<EMPTY>".to_owned())
}

#[inline]
pub fn from_uuid(kind: StoreKind, uuid: uuid::Uuid) -> Self {
Self {
Expand All @@ -150,6 +155,10 @@ impl StoreId {
pub fn as_str(&self) -> &str {
self.id.as_str()
}

pub fn is_empty_recording(&self) -> bool {
self.kind == StoreKind::Recording && self.id.as_str() == "<EMPTY>"
}
}

impl std::fmt::Display for StoreId {
Expand Down
30 changes: 4 additions & 26 deletions crates/re_viewer/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,18 +422,8 @@ impl App {
}
}

SystemCommand::SetSelection { recording_id, item } => {
let recording_id =
recording_id.or_else(|| store_hub.active_recording_id().cloned());
if let Some(recording_id) = recording_id {
if let Some(rec_cfg) = self.state.recording_config_mut(&recording_id) {
rec_cfg.selection_state.set_selection(item);
} else {
re_log::debug!(
"Failed to select item {item:?}: failed to find recording {recording_id}"
);
}
}
SystemCommand::SetSelection(item) => {
self.state.selection_state.set_selection(item);
}

SystemCommand::SetFocus(item) => {
Expand Down Expand Up @@ -576,22 +566,10 @@ impl App {
}

UICommand::SelectionPrevious => {
let state = &mut self.state;
if let Some(rec_cfg) = store_context
.map(|ctx| ctx.recording.store_id())
.and_then(|rec_id| state.recording_config_mut(rec_id))
{
rec_cfg.selection_state.select_previous();
}
self.state.selection_state.select_previous();
}
UICommand::SelectionNext => {
let state = &mut self.state;
if let Some(rec_cfg) = store_context
.map(|ctx| ctx.recording.store_id())
.and_then(|rec_id| state.recording_config_mut(rec_id))
{
rec_cfg.selection_state.select_next();
}
self.state.selection_state.select_next();
}
UICommand::ToggleCommandPalette => {
self.cmd_palette.toggle();
Expand Down
33 changes: 23 additions & 10 deletions crates/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ pub struct AppState {
#[serde(skip)]
viewport_state: ViewportState,

/// Selection & hovering state.
pub selection_state: ApplicationSelectionState,

/// Item that got focused on the last frame if any.
///
/// The focused item is cleared every frame, but views may react with side-effects
Expand All @@ -63,6 +66,7 @@ impl Default for AppState {
blueprint_panel: re_time_panel::TimePanel::new_blueprint_panel(),
welcome_screen: Default::default(),
viewport_state: Default::default(),
selection_state: Default::default(),
focused_item: Default::default(),
}
}
Expand Down Expand Up @@ -125,6 +129,7 @@ impl AppState {
blueprint_panel,
welcome_screen,
viewport_state,
selection_state,
focused_item,
} = self;

Expand Down Expand Up @@ -159,18 +164,21 @@ impl AppState {
return;
}

recording_config_entry(recording_configs, recording.store_id().clone(), recording)
.selection_state
.on_frame_start(
|item| viewport.is_item_valid(item),
re_viewer_context::Item::StoreId(store_context.recording.store_id().clone()),
);
selection_state.on_frame_start(
|item| {
if let re_viewer_context::Item::StoreId(store_id) = item {
if store_id.is_empty_recording() {
return false;
}
}

let rec_cfg =
recording_config_entry(recording_configs, recording.store_id().clone(), recording);
viewport.is_item_valid(item)
},
re_viewer_context::Item::StoreId(store_context.recording.store_id().clone()),
);

if ui.input(|i| i.key_pressed(egui::Key::Escape)) {
rec_cfg.selection_state.clear_selection();
selection_state.clear_selection();
}

let applicable_entities_per_visualizer = space_view_class_registry
Expand Down Expand Up @@ -208,6 +216,9 @@ impl AppState {
.collect::<_>()
};

let rec_cfg =
recording_config_entry(recording_configs, recording.store_id().clone(), recording);

let ctx = ViewerContext {
app_options,
cache,
Expand All @@ -219,6 +230,7 @@ impl AppState {
query_results: &query_results,
rec_cfg,
blueprint_cfg,
selection_state,
blueprint_query: &blueprint_query,
re_ui,
render_ctx,
Expand Down Expand Up @@ -274,6 +286,7 @@ impl AppState {
query_results: &query_results,
rec_cfg,
blueprint_cfg,
selection_state,
blueprint_query: &blueprint_query,
re_ui,
render_ctx,
Expand Down Expand Up @@ -412,7 +425,7 @@ impl AppState {
}

// This must run after any ui code, or other code that tells egui to open an url:
check_for_clicked_hyperlinks(&re_ui.egui_ctx, &rec_cfg.selection_state);
check_for_clicked_hyperlinks(&re_ui.egui_ctx, ctx.selection_state);

// Reset the focused item.
*focused_item = None;
Expand Down
8 changes: 1 addition & 7 deletions crates/re_viewer_context/src/command_sender.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,7 @@ pub enum SystemCommand {
EnableExperimentalDataframeSpaceView(bool),

/// Set the item selection.
SetSelection {
/// If set, use the recording config of this recording.
/// Else, use the currently active recording.
recording_id: Option<StoreId>,

item: crate::Item,
},
SetSelection(crate::Item),

/// Sets the focus to the given item.
///
Expand Down
7 changes: 1 addition & 6 deletions crates/re_viewer_context/src/store_hub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,7 @@ impl StoreHub {
/// matching [`ApplicationId`].
pub fn read_context(&mut self) -> Option<StoreContext<'_>> {
static EMPTY_ENTITY_DB: once_cell::sync::Lazy<EntityDb> =
once_cell::sync::Lazy::new(|| {
EntityDb::new(re_log_types::StoreId::from_string(
StoreKind::Recording,
"<EMPTY>".to_owned(),
))
});
once_cell::sync::Lazy::new(|| EntityDb::new(re_log_types::StoreId::empty_recording()));

// If we have an app-id, then use it to look up the blueprint.
let app_id = self.active_application_id.clone()?;
Expand Down
12 changes: 6 additions & 6 deletions crates/re_viewer_context/src/viewer_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ pub struct ViewerContext<'a> {
/// UI config for the current blueprint.
pub blueprint_cfg: &'a RecordingConfig,

/// Selection & hovering state.
pub selection_state: &'a ApplicationSelectionState,

/// The blueprint query used for resolving blueprint in this frame
pub blueprint_query: &'a LatestAtQuery,

Expand Down Expand Up @@ -89,16 +92,16 @@ impl<'a> ViewerContext<'a> {

/// Returns the current selection.
pub fn selection(&self) -> &ItemCollection {
self.rec_cfg.selection_state.selected_items()
self.selection_state.selected_items()
}

/// Returns the currently hovered objects.
pub fn hovered(&self) -> &ItemCollection {
self.rec_cfg.selection_state.hovered_items()
self.selection_state.hovered_items()
}

pub fn selection_state(&self) -> &ApplicationSelectionState {
&self.rec_cfg.selection_state
self.selection_state
}

/// The current time query, based on the current time control.
Expand Down Expand Up @@ -146,7 +149,4 @@ impl<'a> ViewerContext<'a> {
pub struct RecordingConfig {
/// The current time of the time panel, how fast it is moving, etc.
pub time_ctrl: RwLock<TimeControl>,

/// Selection & hovering state.
pub selection_state: ApplicationSelectionState,
}

0 comments on commit a671f02

Please sign in to comment.