From ea63a888b62bccc9814cc5dc921d607b3b0543f2 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 3 Jan 2024 10:08:28 -0500 Subject: [PATCH] Fix group selection/hover with data queries (#4643) ### What - Resolves: https://github.com/rerun-io/rerun/issues/4377 Restores the behavior of group hover/selection using the cached data query result. Also removes some deprecated TODOs. ### 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/4643/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4643/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/4643/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 - [PR Build Summary](https://build.rerun.io/pr/4643) - [Docs preview](https://rerun.io/preview/cfc9311cc36a57c15ac46eede644d60e67b5d050/docs) - [Examples preview](https://rerun.io/preview/cfc9311cc36a57c15ac46eede644d60e67b5d050/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- crates/re_viewer_context/src/query_context.rs | 11 +++ crates/re_viewport/src/space_view.rs | 1 - .../re_viewport/src/space_view_heuristics.rs | 3 - .../re_viewport/src/space_view_highlights.rs | 78 +++++++++---------- crates/re_viewport/src/system_execution.rs | 3 +- crates/re_viewport/src/viewport.rs | 6 +- 6 files changed, 49 insertions(+), 53 deletions(-) diff --git a/crates/re_viewer_context/src/query_context.rs b/crates/re_viewer_context/src/query_context.rs index 8e8d1d232210..2d457b1390ae 100644 --- a/crates/re_viewer_context/src/query_context.rs +++ b/crates/re_viewer_context/src/query_context.rs @@ -150,6 +150,17 @@ impl DataResultTree { } } + /// Depth-first traversal of a subtree, starting with the given group entity-path, calling `visitor` on each result. + pub fn visit_group( + &self, + entity_path: &EntityPath, + visitor: &mut impl FnMut(DataResultHandle), + ) { + if let Some(subtree_handle) = self.data_results_by_path.get(&(entity_path.clone(), true)) { + self.visit_recursive(*subtree_handle, visitor); + } + } + /// Look up a [`DataResult`] in the tree based on its handle. pub fn lookup_result(&self, handle: DataResultHandle) -> Option<&DataResult> { self.data_results.get(handle).map(|node| &node.data_result) diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index d2987b9dd0e6..69bef57f753c 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -251,7 +251,6 @@ impl SpaceViewBlueprint { let query_result = ctx.lookup_query_result(self.query_id()).clone(); - // TODO(#4377): Use PerSystemDataResults let mut per_system_entities = PerSystemEntities::default(); { re_tracing::profile_scope!("per_system_data_results"); diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 1b5452a9b9f7..e80e57c33151 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -80,8 +80,6 @@ pub fn all_possible_space_views( let mut entity_path_filter = EntityPathFilter::default(); entity_path_filter.add_subtree(candidate_space_path.clone()); - // TODO(#4377): The need to run a query-per-candidate for all possible candidates - // is way too expensive. This needs to be optimized significantly. let candidate_query = DataQueryBlueprint::new(class_identifier, entity_path_filter); @@ -217,7 +215,6 @@ pub fn default_created_space_views( // Main pass through all candidates. // We first check if a candidate is "interesting" and then split it up/modify it further if required. for (candidate, query_result) in candidates { - // TODO(#4377): Can spawn heuristics consume the query_result directly? let mut per_system_entities = PerSystemEntities::default(); { re_tracing::profile_scope!("per_system_data_results"); diff --git a/crates/re_viewport/src/space_view_highlights.rs b/crates/re_viewport/src/space_view_highlights.rs index 6b944c1f58f6..16fd791a261c 100644 --- a/crates/re_viewport/src/space_view_highlights.rs +++ b/crates/re_viewport/src/space_view_highlights.rs @@ -1,21 +1,16 @@ -use std::collections::BTreeMap; - use egui::NumExt; use nohash_hasher::IntMap; use re_log_types::EntityPathHash; use re_renderer::OutlineMaskPreference; use re_viewer_context::{ - ApplicationSelectionState, HoverHighlight, Item, SelectionHighlight, SpaceViewEntityHighlight, - SpaceViewHighlights, SpaceViewId, SpaceViewOutlineMasks, + HoverHighlight, Item, SelectionHighlight, SpaceViewEntityHighlight, SpaceViewHighlights, + SpaceViewId, SpaceViewOutlineMasks, }; -use crate::SpaceViewBlueprint; - pub fn highlights_for_space_view( - selection_state: &ApplicationSelectionState, + ctx: &re_viewer_context::ViewerContext<'_>, space_view_id: SpaceViewId, - _space_views: &BTreeMap, ) -> SpaceViewHighlights { re_tracing::profile_function!(); @@ -36,36 +31,36 @@ pub fn highlights_for_space_view( OutlineMaskPreference::some(hover_mask_index, 0) }; - for current_selection in selection_state.current().iter_items() { + for current_selection in ctx.selection_state().current().iter_items() { match current_selection { Item::ComponentPath(_) | Item::SpaceView(_) | Item::Container(_) => {} - Item::DataBlueprintGroup(_space_view_id, _query_id, _entity_path) => { - // TODO(#4377): Fix DataBlueprintGroup - /* + Item::DataBlueprintGroup(group_space_view_id, query_id, group_entity_path) => { + // Unlike for selected objects/data we are more picky for data blueprints with our hover highlights + // since they are truly local to a space view. if *group_space_view_id == space_view_id { - if let Some(space_view) = space_views.get(group_space_view_id) { - // Everything in the same group should receive the same selection outline. - // (Due to the way outline masks work in re_renderer, we can't leave the hover channel empty) - let selection_mask = next_selection_mask(); - - space_view.contents.visit_group_entities_recursively( - *group_handle, - &mut |entity_path: &EntityPath| { + // Everything in the same group should receive the same selection outline. + // (Due to the way outline masks work in re_renderer, we can't leave the hover channel empty) + let selection_mask = next_selection_mask(); + + let query_result = ctx.lookup_query_result(*query_id).clone(); + + query_result + .tree + .visit_group(group_entity_path, &mut |handle| { + if let Some(result) = query_result.tree.lookup_result(handle) { highlighted_entity_paths - .entry(entity_path.hash()) + .entry(result.entity_path.hash()) .or_default() .overall .selection = SelectionHighlight::SiblingSelection; let outline_mask_ids = - outlines_masks.entry(entity_path.hash()).or_default(); + outlines_masks.entry(result.entity_path.hash()).or_default(); outline_mask_ids.overall = selection_mask.with_fallback_to(outline_mask_ids.overall); - }, - ); - } + } + }); } - */ } Item::InstancePath(selected_space_view_context, selected_instance) => { @@ -113,35 +108,34 @@ pub fn highlights_for_space_view( }; } - for current_hover in selection_state.hovered().iter_items() { + for current_hover in ctx.selection_state().hovered().iter_items() { match current_hover { Item::ComponentPath(_) | Item::SpaceView(_) | Item::Container(_) => {} - Item::DataBlueprintGroup(_space_view_id, _query_id, _entity_path) => { - // TODO(#4377): Fix DataBlueprintGroup - /* + Item::DataBlueprintGroup(group_space_view_id, query_id, group_entity_path) => { // Unlike for selected objects/data we are more picky for data blueprints with our hover highlights // since they are truly local to a space view. if *group_space_view_id == space_view_id { - if let Some(space_view) = space_views.get(group_space_view_id) { - // Everything in the same group should receive the same selection outline. - let hover_mask = next_hover_mask(); + // Everything in the same group should receive the same hover outline. + let hover_mask = next_hover_mask(); + + let query_result = ctx.lookup_query_result(*query_id).clone(); - space_view.contents.visit_group_entities_recursively( - *group_handle, - &mut |entity_path: &EntityPath| { + query_result + .tree + .visit_group(group_entity_path, &mut |handle| { + if let Some(result) = query_result.tree.lookup_result(handle) { highlighted_entity_paths - .entry(entity_path.hash()) + .entry(result.entity_path.hash()) .or_default() .overall .hover = HoverHighlight::Hovered; - let mask = outlines_masks.entry(entity_path.hash()).or_default(); + let mask = + outlines_masks.entry(result.entity_path.hash()).or_default(); mask.overall = hover_mask.with_fallback_to(mask.overall); - }, - ); - } + } + }); } - */ } Item::InstancePath(_, selected_instance) => { diff --git a/crates/re_viewport/src/system_execution.rs b/crates/re_viewport/src/system_execution.rs index 72c9300ac867..38627fb52d9a 100644 --- a/crates/re_viewport/src/system_execution.rs +++ b/crates/re_viewport/src/system_execution.rs @@ -73,8 +73,7 @@ pub fn execute_systems_for_space_views<'a>( .par_drain(..) .filter_map(|space_view_id| { let space_view_blueprint = space_views.get(&space_view_id)?; - let highlights = - highlights_for_space_view(ctx.selection_state(), space_view_id, space_views); + let highlights = highlights_for_space_view(ctx, space_view_id); let output = space_view_blueprint.execute_systems(ctx, time_int, highlights); Some((space_view_id, output)) }) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 3808808446a4..c580f17fe8ef 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -495,11 +495,7 @@ impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { if let Some(result) = self.executed_systems_per_space_view.remove(space_view_id) { result } else { - let highlights = highlights_for_space_view( - self.ctx.selection_state(), - *space_view_id, - self.space_views, - ); + let highlights = highlights_for_space_view(self.ctx, *space_view_id); space_view_blueprint.execute_systems(self.ctx, latest_at, highlights) };