From 89406db2e02d08a203d9ef5c5f3e470ac39f5f72 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 29 Jan 2025 17:16:32 +0100 Subject: [PATCH 1/2] Make less stuff `pub` --- crates/viewer/re_selection_panel/src/selection_panel.rs | 2 +- .../viewer/re_selection_panel/src/view_entity_picker.rs | 2 +- crates/viewer/re_viewport_blueprint/src/view.rs | 2 +- crates/viewer/re_viewport_blueprint/src/view_contents.rs | 8 ++++++-- .../re_viewport_blueprint/src/viewport_blueprint.rs | 4 ++-- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/viewer/re_selection_panel/src/selection_panel.rs b/crates/viewer/re_selection_panel/src/selection_panel.rs index 7222ccda832b..4b697ba074f5 100644 --- a/crates/viewer/re_selection_panel/src/selection_panel.rs +++ b/crates/viewer/re_selection_panel/src/selection_panel.rs @@ -383,7 +383,7 @@ The last rule matching `/world/house` is `+ /world/**`, so it is included. ctx, ui, *view_id, - &view.contents.entity_path_filter, + view.contents.entity_path_filter(), &view.space_origin, ) { view.contents diff --git a/crates/viewer/re_selection_panel/src/view_entity_picker.rs b/crates/viewer/re_selection_panel/src/view_entity_picker.rs index 65cebadc678d..89b98e24e05c 100644 --- a/crates/viewer/re_selection_panel/src/view_entity_picker.rs +++ b/crates/viewer/re_selection_panel/src/view_entity_picker.rs @@ -62,7 +62,7 @@ fn add_entities_ui(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, view: &ViewBluepr let tree = &ctx.recording().tree(); let query_result = ctx.lookup_query_result(view.id); - let entity_path_filter = &view.contents.entity_path_filter; + let entity_path_filter = view.contents.entity_path_filter(); let entities_add_info = create_entity_add_info(ctx, tree, view, query_result); list_item::list_item_scope(ui, "view_entity_picker", |ui| { diff --git a/crates/viewer/re_viewport_blueprint/src/view.rs b/crates/viewer/re_viewport_blueprint/src/view.rs index 84e14ca6c4a4..53ee80b87714 100644 --- a/crates/viewer/re_viewport_blueprint/src/view.rs +++ b/crates/viewer/re_viewport_blueprint/src/view.rs @@ -287,7 +287,7 @@ impl ViewBlueprint { let contents = ViewContents::new( new_id, self.class_identifier, - self.contents.entity_path_filter.clone(), + self.contents.entity_path_filter().clone(), ); Self { diff --git a/crates/viewer/re_viewport_blueprint/src/view_contents.rs b/crates/viewer/re_viewport_blueprint/src/view_contents.rs index 45e50be78a20..9ee21fd812b1 100644 --- a/crates/viewer/re_viewport_blueprint/src/view_contents.rs +++ b/crates/viewer/re_viewport_blueprint/src/view_contents.rs @@ -37,8 +37,8 @@ use crate::{ViewBlueprint, ViewProperty}; pub struct ViewContents { pub blueprint_entity_path: EntityPath, - pub view_class_identifier: ViewClassIdentifier, - pub entity_path_filter: ResolvedEntityPathFilter, + view_class_identifier: ViewClassIdentifier, + entity_path_filter: ResolvedEntityPathFilter, } impl ViewContents { @@ -142,6 +142,10 @@ impl ViewContents { ); } + pub fn entity_path_filter(&self) -> &ResolvedEntityPathFilter { + &self.entity_path_filter + } + /// Set the entity path filter. WARNING: a single mutation is allowed per frame, or data will be /// lost. //TODO(#8518): address this massive foot-gun. diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index e95bb97ffdd7..793c4a7d5020 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -268,7 +268,7 @@ impl ViewportBlueprint { || entity_path.is_descendant_of(&view.space_origin) || view .contents - .entity_path_filter + .entity_path_filter() .matches(&instance_path.entity_path) }) } @@ -359,7 +359,7 @@ impl ViewportBlueprint { .views .values() .filter(|view| view.class_identifier() == class_id) - .map(|view| &view.contents.entity_path_filter) + .map(|view| view.contents.entity_path_filter()) .collect::>(); recommended_views.retain(|(query_filter, _)| { existing_path_filters From 9f2e332271feab24a136bedd0c2daba69179a533 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 29 Jan 2025 18:00:40 +0100 Subject: [PATCH 2/2] Make it possible to mutate the view contents multiple time per frame --- .../re_selection_panel/src/selection_panel.rs | 7 +- .../src/view_contents.rs | 125 ++++++++++-------- 2 files changed, 75 insertions(+), 57 deletions(-) diff --git a/crates/viewer/re_selection_panel/src/selection_panel.rs b/crates/viewer/re_selection_panel/src/selection_panel.rs index 4b697ba074f5..ff4940c914da 100644 --- a/crates/viewer/re_selection_panel/src/selection_panel.rs +++ b/crates/viewer/re_selection_panel/src/selection_panel.rs @@ -7,7 +7,7 @@ use re_data_ui::{ DataUi, }; use re_entity_db::{EntityPath, InstancePath}; -use re_log_types::{ComponentPath, EntityPathFilter, ResolvedEntityPathFilter}; +use re_log_types::{ComponentPath, EntityPathFilter, EntityPathSubs, ResolvedEntityPathFilter}; use re_types::blueprint::components::Interactive; use re_ui::{ icons, @@ -386,8 +386,9 @@ The last rule matching `/world/house` is `+ /world/**`, so it is included. view.contents.entity_path_filter(), &view.space_origin, ) { - view.contents - .set_entity_path_filter(ctx, &new_entity_path_filter); + let path_subs = EntityPathSubs::new_with_origin(&view.space_origin); + let query_filter = new_entity_path_filter.resolve_forgiving(&path_subs); + view.contents.set_entity_path_filter(ctx, query_filter); } }) .header_response diff --git a/crates/viewer/re_viewport_blueprint/src/view_contents.rs b/crates/viewer/re_viewport_blueprint/src/view_contents.rs index 9ee21fd812b1..7a1ffa6675a6 100644 --- a/crates/viewer/re_viewport_blueprint/src/view_contents.rs +++ b/crates/viewer/re_viewport_blueprint/src/view_contents.rs @@ -1,4 +1,7 @@ +use std::sync::Arc; + use nohash_hasher::{IntMap, IntSet}; +use parking_lot::Mutex; use re_log_types::{ResolvedEntityPathFilter, ResolvedEntityPathRule}; use slotmap::SlotMap; use smallvec::SmallVec; @@ -38,7 +41,16 @@ pub struct ViewContents { pub blueprint_entity_path: EntityPath, view_class_identifier: ViewClassIdentifier, + + /// Deserialized entity path filter. + /// + /// Consider this read-only. entity_path_filter: ResolvedEntityPathFilter, + + /// Update entity path filter. + /// + /// Mutations go to this value and should be saved to the blueprint store when they occur. + new_entity_path_filter: Arc>, } impl ViewContents { @@ -83,10 +95,13 @@ impl ViewContents { blueprint_archetypes::ViewContents::name().short_name(), )); + let new_entity_path_filter = Arc::new(Mutex::new(entity_path_filter.clone())); + Self { blueprint_entity_path, view_class_identifier, entity_path_filter, + new_entity_path_filter, } } @@ -120,10 +135,13 @@ impl ViewContents { let entity_path_filter = EntityPathFilter::from_query_expressions(query).resolve_forgiving(subst_env); + let new_entity_path_filter = Arc::new(Mutex::new(entity_path_filter.clone())); + Self { blueprint_entity_path: property.blueprint_store_path, view_class_identifier, entity_path_filter, + new_entity_path_filter, } } @@ -137,7 +155,9 @@ impl ViewContents { ctx.save_blueprint_archetype( &self.blueprint_entity_path, &blueprint_archetypes::ViewContents::new( - self.entity_path_filter.iter_unresolved_expressions(), + self.new_entity_path_filter + .lock() + .iter_unresolved_expressions(), ), ); } @@ -146,27 +166,6 @@ impl ViewContents { &self.entity_path_filter } - /// Set the entity path filter. WARNING: a single mutation is allowed per frame, or data will be - /// lost. - //TODO(#8518): address this massive foot-gun. - pub fn set_entity_path_filter( - &self, - ctx: &ViewerContext<'_>, - new_entity_path_filter: &EntityPathFilter, - ) { - if &self.entity_path_filter.unresolved() == new_entity_path_filter { - return; - } - - ctx.save_blueprint_component( - &self.blueprint_entity_path, - &new_entity_path_filter - .iter_expressions() - .map(|s| QueryExpression(s.into())) - .collect::>(), - ); - } - pub fn build_resolver<'a>( &self, view_class_registry: &'a re_viewer_context::ViewClassRegistry, @@ -191,69 +190,87 @@ impl ViewContents { } } - /// Perform arbitrary mutation on the entity path filter. WARNING: a single mutation is allowed - /// per frame, or data will be lost. + /// Sets the entity path filter to the provided one. + pub fn set_entity_path_filter( + &self, + ctx: &ViewerContext<'_>, + entity_path_filter: ResolvedEntityPathFilter, + ) { + *self.new_entity_path_filter.lock() = entity_path_filter; + self.save_entity_path_filter_to_blueprint(ctx); + } + + /// Perform arbitrary mutation on the entity path filter. /// - /// This method exists because of the single mutation per frame limitation. It currently is the - /// only way to perform multiple mutations on the entity path filter in a single frame. - //TODO(#8518): address this massive foot-gun. + /// Using this method avoids triggering multiple writes to the blueprint store. pub fn mutate_entity_path_filter( &self, ctx: &ViewerContext<'_>, f: impl FnOnce(&mut ResolvedEntityPathFilter), ) { - let mut new_entity_path_filter = self.entity_path_filter.clone(); - f(&mut new_entity_path_filter); - self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); + f(&mut self.new_entity_path_filter.lock()); + self.save_entity_path_filter_to_blueprint(ctx); } - /// Remove a subtree and any existing rules that it would match. WARNING: a single mutation is - /// allowed per frame, or data will be lost. + /// Remove a subtree and any existing rules that it would match. /// /// Because most-specific matches win, if we only add a subtree exclusion /// it can still be overridden by existing inclusions. This method ensures /// that not only do we add a subtree exclusion, but clear out any existing /// inclusions or (now redundant) exclusions that would match the subtree. - //TODO(#8518): address this massive foot-gun. pub fn remove_subtree_and_matching_rules(&self, ctx: &ViewerContext<'_>, path: EntityPath) { - let mut new_entity_path_filter = self.entity_path_filter.clone(); - new_entity_path_filter.remove_subtree_and_matching_rules(path); - self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); + self.new_entity_path_filter + .lock() + .remove_subtree_and_matching_rules(path); + + self.save_entity_path_filter_to_blueprint(ctx); } - /// Directly add an exclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is - /// allowed per frame, or data will be lost. + /// Directly add an exclusion rule to the [`EntityPathFilter`]. /// /// This is a direct modification of the filter and will not do any simplification /// related to overlapping or conflicting rules. /// /// If you are trying to remove an entire subtree, prefer using [`Self::remove_subtree_and_matching_rules`]. - //TODO(#8518): address this massive foot-gun. pub fn raw_add_entity_exclusion(&self, ctx: &ViewerContext<'_>, rule: ResolvedEntityPathRule) { - let mut new_entity_path_filter = self.entity_path_filter.clone(); - new_entity_path_filter.add_rule(RuleEffect::Exclude, rule); - self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); + self.new_entity_path_filter + .lock() + .add_rule(RuleEffect::Exclude, rule); + + self.save_entity_path_filter_to_blueprint(ctx); } - /// Directly add an inclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is - /// allowed per frame, or data will be lost. + /// Directly add an inclusion rule to the [`EntityPathFilter`]. /// /// This is a direct modification of the filter and will not do any simplification /// related to overlapping or conflicting rules. - //TODO(#8518): address this massive foot-gun. pub fn raw_add_entity_inclusion(&self, ctx: &ViewerContext<'_>, rule: ResolvedEntityPathRule) { - let mut new_entity_path_filter = self.entity_path_filter.clone(); - new_entity_path_filter.add_rule(RuleEffect::Include, rule); - self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); + self.new_entity_path_filter + .lock() + .add_rule(RuleEffect::Include, rule); + + self.save_entity_path_filter_to_blueprint(ctx); } - /// Remove rules for a given entity. WARNING: a single mutation is allowed per frame, or data - /// will be lost. - //TODO(#8518): address this massive foot-gun. + /// Remove rules for a given entity. pub fn remove_filter_rule_for(&self, ctx: &ViewerContext<'_>, ent_path: &EntityPath) { - let mut new_entity_path_filter = self.entity_path_filter.clone(); - new_entity_path_filter.remove_rule_for(ent_path); - self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); + self.new_entity_path_filter.lock().remove_rule_for(ent_path); + + self.save_entity_path_filter_to_blueprint(ctx); + } + + /// Save the entity path filter. + fn save_entity_path_filter_to_blueprint(&self, ctx: &ViewerContext<'_>) { + ctx.save_blueprint_component( + &self.blueprint_entity_path, + &self + .new_entity_path_filter + .lock() + .unresolved() + .iter_expressions() + .map(|s| QueryExpression(s.into())) + .collect::>(), + ); } /// Build up the initial [`DataQueryResult`] for this [`ViewContents`]