From a5ed2dfdc854344255b88700cb6432f2ab2a57dc Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 7 Mar 2024 15:36:25 +0100 Subject: [PATCH] fix previously filtered recommendations showing up later again --- .../src/path/entity_path_filter.rs | 6 ++ .../archetypes/viewport_blueprint.fbs | 4 +- .../src/space_view/spawn_heuristics.rs | 2 +- .../archetypes/viewport_blueprint.rs | 26 +++--- crates/re_viewport/src/viewport_blueprint.rs | 83 +++++++++++-------- .../archetypes/viewport_blueprint.cpp | 4 +- .../archetypes/viewport_blueprint.hpp | 12 +-- .../archetypes/viewport_blueprint.py | 14 ++-- .../tests/unit/test_viewport_blueprint.py | 12 +-- 9 files changed, 91 insertions(+), 72 deletions(-) diff --git a/crates/re_log_types/src/path/entity_path_filter.rs b/crates/re_log_types/src/path/entity_path_filter.rs index 9f938629f976..557999d0184a 100644 --- a/crates/re_log_types/src/path/entity_path_filter.rs +++ b/crates/re_log_types/src/path/entity_path_filter.rs @@ -46,6 +46,12 @@ impl std::hash::Hash for EntityPathFilter { } } +impl std::fmt::Debug for EntityPathFilter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "EntityPathFilter({:?})", self.formatted()) + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct EntityPathRule { pub path: EntityPath, diff --git a/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs b/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs index 9f7cde1e38cf..a2d5ebe8a40d 100644 --- a/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs +++ b/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs @@ -39,7 +39,7 @@ table ViewportBlueprint ( /// /// True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover /// all logged entities appropriately, it will do so unless they were added previously - /// (as identified by `viewer_recommendation_hashes`). + /// (as identified by `past_viewer_recommendations`). auto_space_views: rerun.blueprint.components.AutoSpaceViews ("attr.rerun.component_optional", nullable, order: 5000); /// Hashes of all recommended space views the viewer has already added and that should not be added again. @@ -48,5 +48,5 @@ table ViewportBlueprint ( /// If you want the viewer from stopping to add space views, you should set `auto_space_views` to `false`. /// /// The viewer uses this to determine whether it should keep adding space views. - viewer_recommendation_hashes: [rerun.blueprint.components.ViewerRecommendationHash] ("attr.rerun.component_optional", nullable, order: 6000); + past_viewer_recommendations: [rerun.blueprint.components.ViewerRecommendationHash] ("attr.rerun.component_optional", nullable, order: 6000); } diff --git a/crates/re_viewer_context/src/space_view/spawn_heuristics.rs b/crates/re_viewer_context/src/space_view/spawn_heuristics.rs index fea9bdd26a59..628987c1603f 100644 --- a/crates/re_viewer_context/src/space_view/spawn_heuristics.rs +++ b/crates/re_viewer_context/src/space_view/spawn_heuristics.rs @@ -1,7 +1,7 @@ use re_log_types::{EntityPath, EntityPathFilter}; /// Properties of a space view that as recommended to be spawned by default via space view spawn heuristics. -#[derive(Hash)] +#[derive(Hash, Debug)] pub struct RecommendedSpaceView { pub root: EntityPath, pub query_filter: EntityPathFilter, diff --git a/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs b/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs index b21337855342..7df5fbc6cf5e 100644 --- a/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs +++ b/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs @@ -43,7 +43,7 @@ pub struct ViewportBlueprint { /// /// True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover /// all logged entities appropriately, it will do so unless they were added previously - /// (as identified by `viewer_recommendation_hashes`). + /// (as identified by `past_viewer_recommendations`). pub auto_space_views: Option, /// Hashes of all recommended space views the viewer has already added and that should not be added again. @@ -52,7 +52,7 @@ pub struct ViewportBlueprint { /// If you want the viewer from stopping to add space views, you should set `auto_space_views` to `false`. /// /// The viewer uses this to determine whether it should keep adding space views. - pub viewer_recommendation_hashes: + pub past_viewer_recommendations: Option>, } @@ -64,7 +64,7 @@ impl ::re_types_core::SizeBytes for ViewportBlueprint { + self.maximized.heap_size_bytes() + self.auto_layout.heap_size_bytes() + self.auto_space_views.heap_size_bytes() - + self.viewer_recommendation_hashes.heap_size_bytes() + + self.past_viewer_recommendations.heap_size_bytes() } #[inline] @@ -214,19 +214,19 @@ impl ::re_types_core::Archetype for ViewportBlueprint { } else { None }; - let viewer_recommendation_hashes = if let Some(array) = + let past_viewer_recommendations = if let Some(array) = arrays_by_name.get("rerun.blueprint.components.ViewerRecommendationHash") { Some({ ::from_arrow_opt(&**array) .with_context( - "rerun.blueprint.archetypes.ViewportBlueprint#viewer_recommendation_hashes", + "rerun.blueprint.archetypes.ViewportBlueprint#past_viewer_recommendations", )? .into_iter() .map(|v| v.ok_or_else(DeserializationError::missing_data)) .collect::>>() .with_context( - "rerun.blueprint.archetypes.ViewportBlueprint#viewer_recommendation_hashes", + "rerun.blueprint.archetypes.ViewportBlueprint#past_viewer_recommendations", )? }) } else { @@ -238,7 +238,7 @@ impl ::re_types_core::Archetype for ViewportBlueprint { maximized, auto_layout, auto_space_views, - viewer_recommendation_hashes, + past_viewer_recommendations, }) } } @@ -262,7 +262,7 @@ impl ::re_types_core::AsComponents for ViewportBlueprint { self.auto_space_views .as_ref() .map(|comp| (comp as &dyn ComponentBatch).into()), - self.viewer_recommendation_hashes + self.past_viewer_recommendations .as_ref() .map(|comp_batch| (comp_batch as &dyn ComponentBatch).into()), ] @@ -289,7 +289,7 @@ impl ViewportBlueprint { maximized: None, auto_layout: None, auto_space_views: None, - viewer_recommendation_hashes: None, + past_viewer_recommendations: None, } } @@ -330,14 +330,14 @@ impl ViewportBlueprint { } #[inline] - pub fn with_viewer_recommendation_hashes( + pub fn with_past_viewer_recommendations( mut self, - viewer_recommendation_hashes: impl IntoIterator< + past_viewer_recommendations: impl IntoIterator< Item = impl Into, >, ) -> Self { - self.viewer_recommendation_hashes = Some( - viewer_recommendation_hashes + self.past_viewer_recommendations = Some( + past_viewer_recommendations .into_iter() .map(Into::into) .collect(), diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index d67cb6bfe843..b06078d60f69 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -58,7 +58,7 @@ pub struct ViewportBlueprint { auto_space_views: AtomicBool, /// Hashes of all recommended space views the viewer has already added and that should not be added again. - viewer_recommendation_hashes: IntSet, + past_viewer_recommendation_hashes: IntSet, /// Channel to pass Blueprint mutation messages back to the [`crate::Viewport`] tree_action_sender: std::sync::mpsc::Sender, @@ -79,7 +79,7 @@ impl ViewportBlueprint { maximized, auto_layout, auto_space_views, - viewer_recommendation_hashes, + past_viewer_recommendations: past_viewer_recommendation_hashes, } = match query_archetype(blueprint_db.store(), query, &VIEWPORT_PATH.into()) .and_then(|arch| arch.to_archetype()) { @@ -144,7 +144,7 @@ impl ViewportBlueprint { root_container, ); - let viewer_recommendation_hashes = viewer_recommendation_hashes + let past_viewer_recommendation_hashes = past_viewer_recommendation_hashes .unwrap_or_default() .into_iter() .map(|h| Hash64::from_u64(h.0 .0)) @@ -158,7 +158,7 @@ impl ViewportBlueprint { maximized: maximized.map(|id| id.0.into()), auto_layout: auto_layout.unwrap_or_default().0.into(), auto_space_views: auto_space_views.into(), - viewer_recommendation_hashes, + past_viewer_recommendation_hashes, tree_action_sender, } } @@ -296,10 +296,39 @@ impl ViewportBlueprint { // Remove all space views that we already spawned via heuristic before. recommended_space_views.retain(|recommended_view| { !self - .viewer_recommendation_hashes + .past_viewer_recommendation_hashes .contains(&Hash64::hash(recommended_view)) }); + // Each of the remaining recommendations would individually be a candidate for spawning if there were + // no other space views in the viewport. + // In the following steps we further filter this list depending on what's on screen already, + // as well as redundancy within the recommendation itself BUT this is an important checkpoint: + // All the other views may change due to user interaction, but this does *not* mean + // that we should suddenly spawn the views we're filtering out here. + // Therefore everything so far needs to be added to `past_viewer_recommendations`, + // which marks this as "already processed recommendation". + // + // Example: + // Recommendation contains `/**` and `/camera/**`. + // We filter out `/camera/**` because that would be redundant to `/**`. + // If now the user edits the space view at `/**` to be `/points/**`, that does *not* + // mean we should suddenly add `/camera/**` to the viewport. + if !recommended_space_views.is_empty() { + let new_viewer_recommendation_hashes = self + .past_viewer_recommendation_hashes + .iter() + .cloned() + .chain(recommended_space_views.iter().map(Hash64::hash)) + .map(|hash| ViewerRecommendationHash(hash.hash64().into())) + .collect::>(); + + ctx.save_blueprint_component( + &VIEWPORT_PATH.into(), + &new_viewer_recommendation_hashes, + ); + } + // Remove all space views that have all the entities we already have on screen. let existing_path_filters = self .space_views @@ -326,36 +355,20 @@ impl ViewportBlueprint { i == *j || !other.query_filter.is_superset_of(&candidate.query_filter) }) }) - .map(|(_, recommendation)| recommendation) - .collect::>(); - - if !final_recommendations.is_empty() { - self.add_space_views( - final_recommendations.iter().map(|recommendation| { - SpaceViewBlueprint::new( - class_id, - &recommendation.root, - recommendation.query_filter.clone(), - ) - }), - ctx, - None, - None, - ); - - // Finally, save the added recommendations to the store. - let new_viewer_recommendation_hashes = self - .viewer_recommendation_hashes - .iter() - .cloned() - .chain(final_recommendations.iter().map(Hash64::hash)) - .map(|hash| ViewerRecommendationHash(hash.hash64().into())) - .collect::>(); - ctx.save_blueprint_component( - &VIEWPORT_PATH.into(), - &new_viewer_recommendation_hashes, - ); - } + .map(|(_, recommendation)| recommendation); + + self.add_space_views( + final_recommendations.map(|recommendation| { + SpaceViewBlueprint::new( + class_id, + &recommendation.root, + recommendation.query_filter.clone(), + ) + }), + ctx, + None, + None, + ); } } diff --git a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp index 3844a09c746f..e4f142f58638 100644 --- a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp +++ b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp @@ -41,8 +41,8 @@ namespace rerun { RR_RETURN_NOT_OK(result.error); cells.push_back(std::move(result.value)); } - if (archetype.viewer_recommendation_hashes.has_value()) { - auto result = DataCell::from_loggable(archetype.viewer_recommendation_hashes.value()); + if (archetype.past_viewer_recommendations.has_value()) { + auto result = DataCell::from_loggable(archetype.past_viewer_recommendations.value()); RR_RETURN_NOT_OK(result.error); cells.push_back(std::move(result.value)); } diff --git a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp index 4ae3dba42f4f..8ab6cd43ee9a 100644 --- a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp +++ b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp @@ -42,7 +42,7 @@ namespace rerun::blueprint::archetypes { /// /// True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover /// all logged entities appropriately, it will do so unless they were added previously - /// (as identified by `viewer_recommendation_hashes`). + /// (as identified by `past_viewer_recommendations`). std::optional auto_space_views; /// Hashes of all recommended space views the viewer has already added and that should not be added again. @@ -52,7 +52,7 @@ namespace rerun::blueprint::archetypes { /// /// The viewer uses this to determine whether it should keep adding space views. std::optional> - viewer_recommendation_hashes; + past_viewer_recommendations; public: static constexpr const char IndicatorComponentName[] = @@ -102,7 +102,7 @@ namespace rerun::blueprint::archetypes { /// /// True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover /// all logged entities appropriately, it will do so unless they were added previously - /// (as identified by `viewer_recommendation_hashes`). + /// (as identified by `past_viewer_recommendations`). ViewportBlueprint with_auto_space_views( rerun::blueprint::components::AutoSpaceViews _auto_space_views ) && { @@ -117,11 +117,11 @@ namespace rerun::blueprint::archetypes { /// If you want the viewer from stopping to add space views, you should set `auto_space_views` to `false`. /// /// The viewer uses this to determine whether it should keep adding space views. - ViewportBlueprint with_viewer_recommendation_hashes( + ViewportBlueprint with_past_viewer_recommendations( Collection - _viewer_recommendation_hashes + _past_viewer_recommendations ) && { - viewer_recommendation_hashes = std::move(_viewer_recommendation_hashes); + past_viewer_recommendations = std::move(_past_viewer_recommendations); // See: https://github.com/rerun-io/rerun/issues/4027 RR_WITH_MAYBE_UNINITIALIZED_DISABLED(return std::move(*this);) } diff --git a/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py b/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py index 7f22f4e04ab2..4fb9aa248493 100644 --- a/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py +++ b/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py @@ -29,7 +29,7 @@ def __init__( maximized: datatypes.UuidLike | None = None, auto_layout: blueprint_components.AutoLayoutLike | None = None, auto_space_views: blueprint_components.AutoSpaceViewsLike | None = None, - viewer_recommendation_hashes: datatypes.UInt64ArrayLike | None = None, + past_viewer_recommendations: datatypes.UInt64ArrayLike | None = None, ): """ Create a new instance of the ViewportBlueprint archetype. @@ -52,8 +52,8 @@ def __init__( True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover all logged entities appropriately, it will do so unless they were added previously - (as identified by `viewer_recommendation_hashes`). - viewer_recommendation_hashes: + (as identified by `past_viewer_recommendations`). + past_viewer_recommendations: Hashes of all recommended space views the viewer has already added and that should not be added again. This is an internal field and should not be set usually. @@ -71,7 +71,7 @@ def __init__( maximized=maximized, auto_layout=auto_layout, auto_space_views=auto_space_views, - viewer_recommendation_hashes=viewer_recommendation_hashes, + past_viewer_recommendations=past_viewer_recommendations, ) return self.__attrs_clear__() @@ -84,7 +84,7 @@ def __attrs_clear__(self) -> None: maximized=None, # type: ignore[arg-type] auto_layout=None, # type: ignore[arg-type] auto_space_views=None, # type: ignore[arg-type] - viewer_recommendation_hashes=None, # type: ignore[arg-type] + past_viewer_recommendations=None, # type: ignore[arg-type] ) @classmethod @@ -141,11 +141,11 @@ def _clear(cls) -> ViewportBlueprint: # # True if not specified, meaning that if the Viewer deems it necessary to add new Space Views to cover # all logged entities appropriately, it will do so unless they were added previously - # (as identified by `viewer_recommendation_hashes`). + # (as identified by `past_viewer_recommendations`). # # (Docstring intentionally commented out to hide this field from the docs) - viewer_recommendation_hashes: blueprint_components.ViewerRecommendationHashBatch | None = field( + past_viewer_recommendations: blueprint_components.ViewerRecommendationHashBatch | None = field( metadata={"component": "optional"}, default=None, converter=blueprint_components.ViewerRecommendationHashBatch._optional, # type: ignore[misc] diff --git a/rerun_py/tests/unit/test_viewport_blueprint.py b/rerun_py/tests/unit/test_viewport_blueprint.py index fb0a54ea6ac3..153ef1ea467a 100644 --- a/rerun_py/tests/unit/test_viewport_blueprint.py +++ b/rerun_py/tests/unit/test_viewport_blueprint.py @@ -52,7 +52,7 @@ def test_viewport_blueprint() -> None: maximized, auto_layout, auto_space_views, - viewer_recommendation_hashes, + past_viewer_recommendations, ) in all_arrays: space_views = space_views if space_views is not None else space_views_arrays[-1] @@ -62,7 +62,7 @@ def test_viewport_blueprint() -> None: maximized = cast(Optional[UuidLike], maximized) auto_layout = cast(Optional[AutoLayoutLike], auto_layout) auto_space_views = cast(Optional[AutoSpaceViewsLike], auto_space_views) - viewer_recommendation_hashes = cast(Optional[UInt64ArrayLike], viewer_recommendation_hashes) + past_viewer_recommendations = cast(Optional[UInt64ArrayLike], past_viewer_recommendations) print( "rr.ViewportBlueprint(\n", @@ -71,7 +71,7 @@ def test_viewport_blueprint() -> None: f" maximized={maximized!r}\n", f" auto_layout={auto_layout!r}\n", f" auto_space_views={auto_space_views!r}\n", - f" viewer_recommendation_hashes={viewer_recommendation_hashes!r}\n", + f" past_viewer_recommendations={past_viewer_recommendations!r}\n", ")", ) arch = ViewportBlueprint( @@ -80,7 +80,7 @@ def test_viewport_blueprint() -> None: maximized=maximized, auto_layout=auto_layout, auto_space_views=auto_space_views, - viewer_recommendation_hashes=viewer_recommendation_hashes, + past_viewer_recommendations=past_viewer_recommendations, ) print(f"{arch}\n") @@ -89,6 +89,6 @@ def test_viewport_blueprint() -> None: assert arch.maximized == SpaceViewMaximizedBatch._optional(none_empty_or_value(maximized, uuid_bytes1)) assert arch.auto_layout == AutoLayoutBatch._optional(none_empty_or_value(auto_layout, True)) assert arch.auto_space_views == AutoSpaceViewsBatch._optional(none_empty_or_value(auto_space_views, False)) - assert arch.viewer_recommendation_hashes == ViewerRecommendationHashBatch._optional( - none_empty_or_value(viewer_recommendation_hashes, [123, 321]) + assert arch.past_viewer_recommendations == ViewerRecommendationHashBatch._optional( + none_empty_or_value(past_viewer_recommendations, [123, 321]) )