From a3f20fe8142c45a1e57e4fc2c0bb7b30f9b4ec74 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Wed, 11 Dec 2024 09:39:32 +0100 Subject: [PATCH 1/4] Better ordering of picking results on hover --- crates/viewer/re_view_spatial/src/picking.rs | 14 ++++++++++++-- crates/viewer/re_view_spatial/src/picking_ui.rs | 5 ++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/picking.rs b/crates/viewer/re_view_spatial/src/picking.rs index fcd12266d257..a9bef4a6c322 100644 --- a/crates/viewer/re_view_spatial/src/picking.rs +++ b/crates/viewer/re_view_spatial/src/picking.rs @@ -39,7 +39,9 @@ pub struct PickingRayHit { #[derive(Clone, PartialEq)] pub struct PickingResult { - /// Picking ray hits. NOT sorted by distance but rather by source of picking. + /// Picking ray hits. + /// + /// The hits are ROUGHLY sorted front-to-back, i.e. closest hits are first. /// /// Typically there is only one hit, but there might be several if there are transparent objects /// or "aggressive" objects like 2D images which we always want to pick, even if they're in the background. @@ -141,7 +143,7 @@ impl PickingContext { // Start with gpu based picking as baseline. This is our prime source of picking information. // // ..unless the same object got also picked as part of a textured rect. - // Textured rect picks also know where on the rect, making this the better source! + // Textured rect picks also know where on the rect they hit, making this the better source! // Note that whenever this happens, it means that the same object path has a textured rect and something else // e.g. a camera. if let Some(gpu_pick) = gpu_pick { @@ -168,6 +170,14 @@ impl PickingContext { .filter(|ui_hit| !previously_hit_objects.contains(&ui_hit.instance_path_hash)), ); + hits.sort_by_key(|hit| match hit.hit_type { + PickingHitType::GuiOverlay => 0, // GUI is closest, so always goes on top + + PickingHitType::GpuPickingResult => 1, + + PickingHitType::TexturedRect => 2, // Images are usually behind other things (e.g. an image is behind a bounding rectangle in a 2D view), so we put these last (furthest last) + }); + PickingResult { hits } } } diff --git a/crates/viewer/re_view_spatial/src/picking_ui.rs b/crates/viewer/re_view_spatial/src/picking_ui.rs index b91ebcdf5c7f..db4ed18b29fe 100644 --- a/crates/viewer/re_view_spatial/src/picking_ui.rs +++ b/crates/viewer/re_view_spatial/src/picking_ui.rs @@ -82,7 +82,10 @@ pub fn picking( // Depth at pointer used for projecting rays from a hovered 2D view to corresponding 3D view(s). // TODO(#1818): Depth at pointer only works for depth images so far. let mut depth_at_pointer = None; - for (hit_idx, hit) in picking_result.hits.iter().enumerate() { + + // We iterate back-to-front, so that we show the furthest hits first, + // i.e. the background before the foreground. + for (hit_idx, hit) in picking_result.hits.iter().rev().enumerate() { let Some(mut instance_path) = hit.instance_path_hash.resolve(ctx.recording()) else { // Entity no longer exists in db. continue; From b7a0e17fe880d1c2637b46204fbc399fa865e04d Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Wed, 11 Dec 2024 09:44:01 +0100 Subject: [PATCH 2/4] Better naming --- crates/viewer/re_view_spatial/src/picking.rs | 31 +++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/picking.rs b/crates/viewer/re_view_spatial/src/picking.rs index a9bef4a6c322..f9c5a09a694f 100644 --- a/crates/viewer/re_view_spatial/src/picking.rs +++ b/crates/viewer/re_view_spatial/src/picking.rs @@ -134,29 +134,31 @@ impl PickingContext { self, previous_picking_result, ); - let mut rect_hits = picking_textured_rects(self, images); - rect_hits.sort_by(|a, b| b.depth_offset.cmp(&a.depth_offset)); - let ui_rect_hits = picking_ui_rects(self, ui_rects); + + let mut image_hits = picking_textured_rects(self, images); + image_hits.sort_by(|a, b| b.depth_offset.cmp(&a.depth_offset)); + + let ui_hits = picking_ui_rects(self, ui_rects); let mut hits = Vec::new(); // Start with gpu based picking as baseline. This is our prime source of picking information. - // - // ..unless the same object got also picked as part of a textured rect. - // Textured rect picks also know where on the rect they hit, making this the better source! - // Note that whenever this happens, it means that the same object path has a textured rect and something else - // e.g. a camera. if let Some(gpu_pick) = gpu_pick { - if rect_hits.iter().all(|rect_hit| { - rect_hit.instance_path_hash.entity_path_hash - != gpu_pick.instance_path_hash.entity_path_hash - }) { + // ..unless the same object got also picked as part of a textured rect. + // Textured rect picks also know where on the rect they hit, making this the better source! + // Note that whenever this happens, it means that the same object path has a textured rect and something else + // e.g. a camera. + let has_image_hit_on_gpu_pick = image_hits.iter().any(|image_hit| { + image_hit.instance_path_hash.entity_path_hash + == gpu_pick.instance_path_hash.entity_path_hash + }); + if !has_image_hit_on_gpu_pick { hits.push(gpu_pick); } } // We never throw away any textured rects, even if they're behind other objects. - hits.extend(rect_hits); + hits.extend(image_hits); // UI rects are overlaid on top, but we don't let them hide other picking results either. // Give any other previous hits precedence. @@ -165,11 +167,12 @@ impl PickingContext { .map(|prev_hit| prev_hit.instance_path_hash) .collect(); hits.extend( - ui_rect_hits + ui_hits .into_iter() .filter(|ui_hit| !previously_hit_objects.contains(&ui_hit.instance_path_hash)), ); + // Re-order so that the closest hits are first: hits.sort_by_key(|hit| match hit.hit_type { PickingHitType::GuiOverlay => 0, // GUI is closest, so always goes on top From 617f6bd15d6f5420b8ce01140c507ff83b841b43 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Wed, 11 Dec 2024 10:22:12 +0100 Subject: [PATCH 3/4] Reverse the order --- crates/viewer/re_view_spatial/src/picking_ui.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/picking_ui.rs b/crates/viewer/re_view_spatial/src/picking_ui.rs index db4ed18b29fe..ebaea5dffc22 100644 --- a/crates/viewer/re_view_spatial/src/picking_ui.rs +++ b/crates/viewer/re_view_spatial/src/picking_ui.rs @@ -83,9 +83,8 @@ pub fn picking( // TODO(#1818): Depth at pointer only works for depth images so far. let mut depth_at_pointer = None; - // We iterate back-to-front, so that we show the furthest hits first, - // i.e. the background before the foreground. - for (hit_idx, hit) in picking_result.hits.iter().rev().enumerate() { + // We iterate front-to-back, putting foreground hits on top, like layers in Photoshop: + for (hit_idx, hit) in picking_result.hits.iter().enumerate() { let Some(mut instance_path) = hit.instance_path_hash.resolve(ctx.recording()) else { // Entity no longer exists in db. continue; From e2880cdfce7e508e0526f47dd435c9c5229c6c62 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 12 Dec 2024 10:28:45 +0100 Subject: [PATCH 4/4] Adjust docstring --- crates/viewer/re_view_spatial/src/picking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_view_spatial/src/picking.rs b/crates/viewer/re_view_spatial/src/picking.rs index f9c5a09a694f..327213499d94 100644 --- a/crates/viewer/re_view_spatial/src/picking.rs +++ b/crates/viewer/re_view_spatial/src/picking.rs @@ -41,7 +41,7 @@ pub struct PickingRayHit { pub struct PickingResult { /// Picking ray hits. /// - /// The hits are ROUGHLY sorted front-to-back, i.e. closest hits are first. + /// The hits are sorted front-to-back, i.e. closest hits are first. /// /// Typically there is only one hit, but there might be several if there are transparent objects /// or "aggressive" objects like 2D images which we always want to pick, even if they're in the background.