From e0e22d411d61b6bbc451988f347eccb55ae1a120 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 29 Nov 2023 15:19:53 +0100 Subject: [PATCH] Resolve unexpected view-partitioning by only bucket images when creating a new 2d view (#4361) ### What - Resolves: https://github.com/rerun-io/rerun/issues/4284 Fixes two issues: - Image bucketing only makes sense for 2d spatial views - The bucket logic would previously trigger even when the aggregate view should have lost out according to the scoring, so we move this into the `should_spawn_new` code-path. ### 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 [app.rerun.io](https://app.rerun.io/pr/4361) (if applicable) * [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/4361) - [Docs preview](https://rerun.io/preview/c996f7d7a26f19ce3c281e25663d39ed8f8f4016/docs) - [Examples preview](https://rerun.io/preview/c996f7d7a26f19ce3c281e25663d39ed8f8f4016/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- .../re_viewport/src/space_view_heuristics.rs | 145 +++++++++--------- 1 file changed, 75 insertions(+), 70 deletions(-) diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index b8d0843ddf17..3aec0ca7a6f6 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -27,6 +27,10 @@ fn is_spatial_class(class: &SpaceViewClassName) -> bool { class.as_str() == "3D" || class.as_str() == "2D" } +fn is_spatial_2d_class(class: &SpaceViewClassName) -> bool { + class.as_str() == "2D" +} + fn spawn_one_space_view_per_entity(class: &SpaceViewClassName) -> bool { // For tensors create one space view for each tensor (even though we're able to stack them in one view) // TODO(emilk): query the actual [`ViewPartSystem`] instead. @@ -264,76 +268,6 @@ pub fn default_created_space_views( continue; } - // Spatial views with images get extra treatment as well. - if is_spatial_class(candidate.class_name()) { - #[derive(Hash, PartialEq, Eq)] - enum ImageBucketing { - BySize((u64, u64)), - ExplicitDrawOrder, - } - - let mut images_by_bucket: HashMap> = HashMap::default(); - - // For this we're only interested in the direct children. - for entity_path in &candidate.contents.root_group().entities { - if let Some(tensor) = - store.query_latest_component::(entity_path, &query) - { - if let Some([height, width, _]) = tensor.image_height_width_channels() { - if store - .query_latest_component::( - entity_path, - &query, - ) - .is_some() - { - // Put everything in the same bucket if it has a draw order. - images_by_bucket - .entry(ImageBucketing::ExplicitDrawOrder) - .or_default() - .push(entity_path.clone()); - } else { - // Otherwise, distinguish buckets by image size. - images_by_bucket - .entry(ImageBucketing::BySize((height, width))) - .or_default() - .push(entity_path.clone()); - } - } - } - } - - if images_by_bucket.len() > 1 { - // If all images end up in the same bucket, proceed as normal. Otherwise stack images as instructed. - for bucket in images_by_bucket.keys() { - // Ignore every image from another bucket. Keep all other entities. - let images_of_different_size = images_by_bucket - .iter() - .filter_map(|(other_bucket, images)| { - (bucket != other_bucket).then_some(images) - }) - .flatten() - .cloned() - .collect::>(); - let entities = candidate - .contents - .entity_paths() - .filter(|path| !images_of_different_size.contains(path)) - .cloned() - .collect_vec(); - - let mut space_view = SpaceViewBlueprint::new( - *candidate.class_name(), - &candidate.space_origin, - entities.iter(), - ); - space_view.entities_determined_by_user = true; // Suppress auto adding of entities. - space_views.push((space_view, AutoSpawnHeuristic::AlwaysSpawn)); - } - continue; - } - } - // TODO(andreas): Interaction of [`AutoSpawnHeuristic`] with above hardcoded heuristics is a bit wonky. // `AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot` means we're competing with other candidates for the same root. @@ -367,6 +301,77 @@ pub fn default_created_space_views( } if should_spawn_new { + // 2D views with images get extra treatment as well. + if is_spatial_2d_class(candidate.class_name()) { + #[derive(Hash, PartialEq, Eq)] + enum ImageBucketing { + BySize((u64, u64)), + ExplicitDrawOrder, + } + + let mut images_by_bucket: HashMap> = + HashMap::default(); + + // For this we're only interested in the direct children. + for entity_path in &candidate.contents.root_group().entities { + if let Some(tensor) = + store.query_latest_component::(entity_path, &query) + { + if let Some([height, width, _]) = tensor.image_height_width_channels() { + if store + .query_latest_component::( + entity_path, + &query, + ) + .is_some() + { + // Put everything in the same bucket if it has a draw order. + images_by_bucket + .entry(ImageBucketing::ExplicitDrawOrder) + .or_default() + .push(entity_path.clone()); + } else { + // Otherwise, distinguish buckets by image size. + images_by_bucket + .entry(ImageBucketing::BySize((height, width))) + .or_default() + .push(entity_path.clone()); + } + } + } + } + + if images_by_bucket.len() > 1 { + // If all images end up in the same bucket, proceed as normal. Otherwise stack images as instructed. + for bucket in images_by_bucket.keys() { + // Ignore every image from another bucket. Keep all other entities. + let images_of_different_size = images_by_bucket + .iter() + .filter_map(|(other_bucket, images)| { + (bucket != other_bucket).then_some(images) + }) + .flatten() + .cloned() + .collect::>(); + let entities = candidate + .contents + .entity_paths() + .filter(|path| !images_of_different_size.contains(path)) + .cloned() + .collect_vec(); + + let mut space_view = SpaceViewBlueprint::new( + *candidate.class_name(), + &candidate.space_origin, + entities.iter(), + ); + space_view.entities_determined_by_user = true; // Suppress auto adding of entities. + space_views.push((space_view, AutoSpawnHeuristic::AlwaysSpawn)); + } + continue; + } + } + space_views.push((candidate, spawn_heuristic)); } } else {