Skip to content

Commit

Permalink
Remove items from selection upon removal consistently (#5643)
Browse files Browse the repository at this point in the history
### What

This was done in various places and somewhat inconsistently. This change
unifies selection & history purging as well as selection fallback.

* Fixes  #5406

### 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/5643/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5643/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/5643/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/5643)
- [Docs
preview](https://rerun.io/preview/9f723b58c2966da96bad42369c51c8d8f77d3c97/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/9f723b58c2966da96bad42369c51c8d8f77d3c97/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
Wumpf authored Mar 22, 2024
1 parent 6efb35b commit 84d19d1
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 39 deletions.
24 changes: 10 additions & 14 deletions crates/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,10 @@ impl AppState {

recording_config_entry(recording_configs, recording.store_id().clone(), recording)
.selection_state
.on_frame_start(|item| viewport.is_item_valid(item));
.on_frame_start(
|item| viewport.is_item_valid(item),
re_viewer_context::Item::StoreId(store_context.recording.store_id().clone()),
);

let rec_cfg =
recording_config_entry(recording_configs, recording.store_id().clone(), recording);
Expand Down Expand Up @@ -297,19 +300,12 @@ impl AppState {
app_blueprint.time_panel_expanded,
);

{
if ctx.selection().is_empty() {
// Make sure something is selected before showing the selection panel.
ctx.selection_state()
.set_selection(re_viewer_context::Item::StoreId(ctx.recording_id().clone()));
}
selection_panel.show_panel(
&ctx,
ui,
&mut viewport,
app_blueprint.selection_panel_expanded,
);
}
selection_panel.show_panel(
&ctx,
ui,
&mut viewport,
app_blueprint.selection_panel_expanded,
);

let central_panel_frame = egui::Frame {
fill: ui.style().visuals.panel_fill,
Expand Down
26 changes: 13 additions & 13 deletions crates/re_viewer_context/src/selection_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,18 +227,29 @@ pub struct ApplicationSelectionState {

impl ApplicationSelectionState {
/// Called at the start of each frame
pub fn on_frame_start(&mut self, item_retain_condition: impl Fn(&Item) -> bool) {
pub fn on_frame_start(
&mut self,
item_retain_condition: impl Fn(&Item) -> bool,
fallback_selection: Item,
) {
// Use a different name so we don't get a collision in puffin.
re_tracing::profile_scope!("SelectionState::on_frame_start");

// Purge history of invalid items.
let history = self.history.get_mut();
history.retain(&item_retain_condition);

// Purge selection of invalid items.
let selection_this_frame = self.selection_this_frame.get_mut();
selection_this_frame.retain(|item, _| item_retain_condition(item));
if selection_this_frame.is_empty() {
*selection_this_frame = ItemCollection::from(fallback_selection);
}

// Hovering needs to be refreshed every frame: If it wasn't hovered last frame, it's no longer hovered!
self.hovered_previous_frame = std::mem::take(self.hovered_this_frame.get_mut());

// Selection in contrast, is sticky!
let selection_this_frame = self.selection_this_frame.get_mut();
if selection_this_frame != &self.selection_previous_frame {
history.update_selection(selection_this_frame);
self.selection_previous_frame = selection_this_frame.clone();
Expand Down Expand Up @@ -286,17 +297,6 @@ impl ApplicationSelectionState {
*self.hovered_this_frame.lock() = hovered.into();
}

/// Remove given items from the selection.
///
/// Has no effect on items that were not selected in the first place.
/// Ignores `ItemSpaceContext`s in the passed collection if any.
pub fn remove_from_selection(&self, items: impl Into<ItemCollection>) {
let removed_items = items.into();
self.selection_this_frame
.lock()
.retain(|item, _| !removed_items.contains_item(item));
}

/// Select passed objects unless already selected in which case they get unselected.
/// If however an object is already selected but now gets passed a *different* selected space context, it stays selected after all
/// but with an updated selected space context!
Expand Down
10 changes: 0 additions & 10 deletions crates/re_viewport/src/context_menu/actions/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,13 @@ impl ContextMenuAction for RemoveAction {
.mark_user_interaction(ctx.viewer_context);
ctx.viewport_blueprint
.remove_contents(Contents::Container(*container_id));
ctx.viewer_context
.selection_state()
.remove_from_selection(Item::Container(*container_id));
}

fn process_space_view(&self, ctx: &ContextMenuContext<'_>, space_view_id: &SpaceViewId) {
ctx.viewport_blueprint
.mark_user_interaction(ctx.viewer_context);
ctx.viewport_blueprint
.remove_contents(Contents::SpaceView(*space_view_id));
ctx.viewer_context
.selection_state()
.remove_from_selection(Item::SpaceView(*space_view_id));
}

fn process_data_result(
Expand All @@ -58,10 +52,6 @@ impl ContextMenuAction for RemoveAction {
ctx.viewer_context,
EntityPathRule::including_subtree(instance_path.entity_path.clone()),
);

ctx.viewer_context
.selection_state()
.remove_from_selection(Item::DataResult(*space_view_id, instance_path.clone()));
}
}
}
13 changes: 11 additions & 2 deletions crates/re_viewport/src/viewport_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,18 @@ impl ViewportBlueprint {
pub fn is_item_valid(&self, item: &Item) -> bool {
match item {
Item::StoreId(_) | Item::ComponentPath(_) | Item::InstancePath(_) => true,
Item::SpaceView(space_view_id) | Item::DataResult(space_view_id, _) => {
self.space_view(space_view_id).is_some()

Item::SpaceView(space_view_id) => self.space_view(space_view_id).is_some(),

Item::DataResult(space_view_id, instance_path) => {
self.space_view(space_view_id).map_or(false, |space_view| {
space_view
.contents
.entity_path_filter
.is_included(&instance_path.entity_path)
})
}

Item::Container(container_id) => self.container(container_id).is_some(),
}
}
Expand Down

0 comments on commit 84d19d1

Please sign in to comment.