From 7c1f618502e4b5966d4d677196128722db131f86 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 23 May 2024 11:29:53 +0200 Subject: [PATCH 1/9] generic reset buttons for view property components --- crates/re_query/src/latest_at/results.rs | 5 +++ crates/re_space_view/src/view_property_ui.rs | 39 ++++++++++++------- .../re_ui/src/list_item/property_content.rs | 27 +++++++++++-- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/crates/re_query/src/latest_at/results.rs b/crates/re_query/src/latest_at/results.rs index 0d1c7d8ea377..c1a2489f7cfc 100644 --- a/crates/re_query/src/latest_at/results.rs +++ b/crates/re_query/src/latest_at/results.rs @@ -46,6 +46,11 @@ impl LatestAtResults { self.components.contains_key(&component_name.into()) } + pub fn contains_non_empty(&self, component_name: impl Into) -> bool { + self.get(component_name) + .map_or(false, |result| result.num_instances() != 0) + } + /// Returns the [`LatestAtComponentResults`] for the specified [`Component`]. #[inline] pub fn get( diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 5801147cccf5..d44937e28ac5 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -47,20 +47,31 @@ pub fn view_property_ui( .interactive(false) .show_flat( ui, - list_item::PropertyContent::new(display_name).value_fn(|_, ui, _| { - ctx.component_ui_registry.edit_ui( - ctx, - ui, - re_viewer_context::UiLayout::List, - blueprint_query, - blueprint_db, - &blueprint_path, - &blueprint_path, - component_results.get_or_empty(*component_name), - component_name, - &0.into(), - ); - }), + list_item::PropertyContent::new(display_name) + .action_button_with_enabled( + &re_ui::icons::RESET, + component_results.contains_non_empty(*component_name), + || { + ctx.save_empty_blueprint_component_name( + &blueprint_path, + *component_name, + ); + }, + ) + .value_fn(|_, ui, _| { + ctx.component_ui_registry.edit_ui( + ctx, + ui, + re_viewer_context::UiLayout::List, + blueprint_query, + blueprint_db, + &blueprint_path, + &blueprint_path, + component_results.get_or_empty(*component_name), + component_name, + &0.into(), + ); + }), ); if let Some(tooltip) = field_info.map(|info| info.documentation) { diff --git a/crates/re_ui/src/list_item/property_content.rs b/crates/re_ui/src/list_item/property_content.rs index 1a00082a3f3a..f469b87d6733 100644 --- a/crates/re_ui/src/list_item/property_content.rs +++ b/crates/re_ui/src/list_item/property_content.rs @@ -11,6 +11,7 @@ type PropertyValueFn<'a> = dyn FnOnce(&ReUi, &mut egui::Ui, egui::style::WidgetV struct PropertyActionButton<'a> { icon: &'static crate::icons::Icon, + enabled: bool, on_click: Box, } @@ -93,8 +94,22 @@ impl<'a> PropertyContent<'a> { // TODO(#6191): accept multiple calls for this function for multiple actions. #[inline] pub fn action_button( + self, + icon: &'static crate::icons::Icon, + on_click: impl FnOnce() + 'a, + ) -> Self { + self.action_button_with_enabled(icon, true, on_click) + } + + /// Right aligned action button. + /// + /// Note: for aesthetics, space is always reserved for the action button. + // TODO(#6191): accept multiple calls for this function for multiple actions. + #[inline] + pub fn action_button_with_enabled( mut self, icon: &'static crate::icons::Icon, + enabled: bool, on_click: impl FnOnce() + 'a, ) -> Self { // TODO(#6191): support multiple action buttons @@ -104,6 +119,7 @@ impl<'a> PropertyContent<'a> { ); self.action_buttons = Some(PropertyActionButton { icon, + enabled, on_click: Box::new(on_click), }); self @@ -349,10 +365,13 @@ impl ListItemContent for PropertyContent<'_> { action_button_rect, egui::Layout::right_to_left(egui::Align::Center), ); - let button_response = re_ui.small_icon_button(&mut child_ui, action_button.icon); - if button_response.clicked() { - (action_button.on_click)(); - } + + child_ui.add_enabled_ui(action_button.enabled, |ui| { + let button_response = re_ui.small_icon_button(ui, action_button.icon); + if button_response.clicked() { + (action_button.on_click)(); + } + }); } } From 74cfe245353b14f8d163345e41f132676b0eaf41 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 23 May 2024 15:50:19 +0200 Subject: [PATCH 2/9] reset button now resets to default blueprint giving the reset button the same semantics as the top right reset button --- crates/re_space_view/src/view_property_ui.rs | 13 ++--- crates/re_viewer/src/ui/override_ui.rs | 2 +- .../src/blueprint_helpers.rs | 51 ++++++++++++++++--- crates/re_viewport_blueprint/src/lib.rs | 2 +- .../src/view_properties.rs | 25 +-------- 5 files changed, 51 insertions(+), 42 deletions(-) diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index d44937e28ac5..50afb05c5b56 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -48,16 +48,9 @@ pub fn view_property_ui( .show_flat( ui, list_item::PropertyContent::new(display_name) - .action_button_with_enabled( - &re_ui::icons::RESET, - component_results.contains_non_empty(*component_name), - || { - ctx.save_empty_blueprint_component_name( - &blueprint_path, - *component_name, - ); - }, - ) + .action_button(&re_ui::icons::RESET, || { + ctx.reset_blueprint_component_by_name(&blueprint_path, *component_name); + }) .value_fn(|_, ui, _| { ctx.component_ui_registry.edit_ui( ctx, diff --git a/crates/re_viewer/src/ui/override_ui.rs b/crates/re_viewer/src/ui/override_ui.rs index ba9ddbcc436a..227e41dd2bcf 100644 --- a/crates/re_viewer/src/ui/override_ui.rs +++ b/crates/re_viewer/src/ui/override_ui.rs @@ -159,7 +159,7 @@ pub fn override_ui( re_ui::list_item::PropertyContent::new(component_name.short_name()) .min_desired_width(150.0) .action_button(&re_ui::icons::CLOSE, || { - ctx.save_empty_blueprint_component_name( + ctx.save_empty_blueprint_component_by_name( &overrides.individual_override_path, *component_name, ); diff --git a/crates/re_viewer_context/src/blueprint_helpers.rs b/crates/re_viewer_context/src/blueprint_helpers.rs index 557b81eb70a7..ec2169f915d5 100644 --- a/crates/re_viewer_context/src/blueprint_helpers.rs +++ b/crates/re_viewer_context/src/blueprint_helpers.rs @@ -60,7 +60,7 @@ impl ViewerContext<'_> { entity_path: &EntityPath, components: &dyn ComponentBatch, ) { - let mut data_cell = match DataCell::from_component_batch(components) { + let data_cell = match DataCell::from_component_batch(components) { Ok(data_cell) => data_cell, Err(err) => { re_log::error_once!( @@ -70,15 +70,20 @@ impl ViewerContext<'_> { return; } }; + + self.save_blueprint_data_cell(entity_path, data_cell); + } + + /// Helper to save a data cell to the blueprint store. + pub fn save_blueprint_data_cell(&self, entity_path: &EntityPath, mut data_cell: DataCell) { data_cell.compute_size_bytes(); - let num_instances = components.num_instances() as u32; let timepoint = self.store_context.blueprint_timepoint_for_writes(); re_log::trace!( "Writing {} components of type {:?} to {:?}", - num_instances, - components.name(), + data_cell.num_instances(), + data_cell.component_name(), entity_path ); @@ -111,15 +116,49 @@ impl ViewerContext<'_> { self.save_blueprint_component(entity_path, &empty); } + /// Resets a blueprint component to the value it had in the default blueprint. + pub fn reset_blueprint_component_by_name( + &self, + entity_path: &EntityPath, + component_name: ComponentName, + ) { + let default_blueprint = self.store_context.default_blueprint; + + if let Some(default_value) = default_blueprint.and_then(|default_blueprint| { + default_blueprint + .latest_at(self.blueprint_query, entity_path, [component_name]) + .get(component_name) + .and_then(|default_value| { + default_value.raw(default_blueprint.resolver(), component_name) + }) + }) { + self.save_blueprint_data_cell( + entity_path, + DataCell::from_arrow(component_name, default_value), + ); + } else { + self.save_empty_blueprint_component_by_name(entity_path, component_name); + } + } + /// Helper to save a component to the blueprint store. - pub fn save_empty_blueprint_component_name( + pub fn save_empty_blueprint_component_by_name( &self, entity_path: &EntityPath, component_name: ComponentName, ) { let blueprint = &self.store_context.blueprint; + + // Don't do anything if the component does not exist (if we don't the datatype lookup may fail). + if !blueprint + .latest_at(self.blueprint_query, entity_path, [component_name]) + .contains(component_name) + { + return; + } + let Some(datatype) = blueprint.store().lookup_datatype(&component_name) else { - re_log::error_once!( + re_log::error!( "Tried to clear a component with unknown type: {}", component_name ); diff --git a/crates/re_viewport_blueprint/src/lib.rs b/crates/re_viewport_blueprint/src/lib.rs index 66ec113ae9b1..7ccb76c787a0 100644 --- a/crates/re_viewport_blueprint/src/lib.rs +++ b/crates/re_viewport_blueprint/src/lib.rs @@ -14,7 +14,7 @@ pub use space_view::SpaceViewBlueprint; pub use space_view_contents::SpaceViewContents; pub use tree_actions::TreeAction; pub use view_properties::{ - edit_blueprint_component, entity_path_for_view_property, get_blueprint_component, + edit_blueprint_component, entity_path_for_view_property, query_view_property, query_view_property_or_default, view_property, }; pub use viewport_blueprint::ViewportBlueprint; diff --git a/crates/re_viewport_blueprint/src/view_properties.rs b/crates/re_viewport_blueprint/src/view_properties.rs index 9a137a3688f1..1b268e35a908 100644 --- a/crates/re_viewport_blueprint/src/view_properties.rs +++ b/crates/re_viewport_blueprint/src/view_properties.rs @@ -70,19 +70,6 @@ where (arch.ok().flatten().unwrap_or_default(), path) } -/// Read a single component of a blueprint archetype in a space view. -pub fn get_blueprint_component( - ctx: &ViewerContext<'_>, - space_view_id: SpaceViewId, -) -> Option { - let blueprint_db = ctx.store_context.blueprint; - let query = ctx.blueprint_query; - let path = entity_path_for_view_property::(space_view_id, blueprint_db.tree()); - blueprint_db - .latest_at_component::(&path, query) - .map(|x| x.value) -} - /// Edit a single component of a blueprint archetype in a space view. /// /// Set to `None` to reset the value to the value in the default blueprint, if any, @@ -106,17 +93,7 @@ pub fn edit_blueprint_component(space_view_id, default_blueprint.tree()); - default_blueprint - .latest_at_component::(&default_path, ctx.blueprint_query) - .map(|x| x.value) - }); - ctx.save_blueprint_component(&active_path, &default_value); + ctx.reset_blueprint_component_by_name(&active_path, C::name()); } } From 17b8a79c2138ee1bf9283fdc783dcb3aba0ca4dd Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 23 May 2024 16:30:21 +0200 Subject: [PATCH 3/9] fix warning on first edit in generic view property archetype ui --- crates/re_data_ui/src/editors/corner2d.rs | 4 +- crates/re_data_ui/src/editors/mod.rs | 56 ++++++++++--------- crates/re_data_ui/src/editors/visible.rs | 4 +- crates/re_space_view/src/view_property_ui.rs | 4 +- crates/re_viewer/src/ui/override_ui.rs | 4 +- .../src/component_ui_registry.rs | 11 ++-- 6 files changed, 43 insertions(+), 40 deletions(-) diff --git a/crates/re_data_ui/src/editors/corner2d.rs b/crates/re_data_ui/src/editors/corner2d.rs index e82af0786e36..de941b641ef6 100644 --- a/crates/re_data_ui/src/editors/corner2d.rs +++ b/crates/re_data_ui/src/editors/corner2d.rs @@ -13,12 +13,12 @@ pub fn edit_corner2d( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let corner = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_corner2d(ctx, query, db, entity_path)); let mut edit_corner = corner; diff --git a/crates/re_data_ui/src/editors/mod.rs b/crates/re_data_ui/src/editors/mod.rs index 4c86de8bd8f0..8173bd07a632 100644 --- a/crates/re_data_ui/src/editors/mod.rs +++ b/crates/re_data_ui/src/editors/mod.rs @@ -27,12 +27,12 @@ fn edit_color_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_color = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_color(ctx, query, db, entity_path)); let current_color = current_color.into(); @@ -70,12 +70,12 @@ fn edit_text_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_text(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -110,12 +110,12 @@ fn edit_name_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_name(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -151,12 +151,12 @@ fn edit_scatter_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_scatter = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_scatter(ctx, query, db, entity_path)); let current_scatter = current_scatter.0; @@ -200,12 +200,12 @@ fn edit_radius_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_radius = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_radius(ctx, query, db, entity_path)); let current_radius = current_radius.0; @@ -247,12 +247,12 @@ fn edit_marker_shape_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_marker = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_marker_shape(ctx, query, db, entity_path)); let mut edit_marker = current_marker; @@ -352,12 +352,12 @@ fn edit_stroke_width_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_stroke_width = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_stroke_width(ctx, query, db, entity_path)); let current_stroke_width = current_stroke_width.0; @@ -399,12 +399,12 @@ fn edit_marker_size_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let current_marker_size = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_marker_size(ctx, query, db, entity_path)); let current_marker_size = current_marker_size.0; @@ -437,20 +437,22 @@ fn default_marker_size( // ---- +type EditCallback = fn( + &ViewerContext<'_>, + &mut egui::Ui, + UiLayout, + &LatestAtQuery, + &EntityDb, + &EntityPath, + &EntityPath, + Option<&LatestAtComponentResults>, + &Instance, +); + fn register_editor<'a, C>( registry: &mut re_viewer_context::ComponentUiRegistry, default: fn(&ViewerContext<'_>, &LatestAtQuery, &EntityDb, &EntityPath) -> C, - edit: fn( - &ViewerContext<'_>, - &mut egui::Ui, - UiLayout, - &LatestAtQuery, - &EntityDb, - &EntityPath, - &EntityPath, - &LatestAtComponentResults, - &Instance, - ), + edit: EditCallback, ) where C: Component + Loggable + 'static + Into<::std::borrow::Cow<'a, C>>, { diff --git a/crates/re_data_ui/src/editors/visible.rs b/crates/re_data_ui/src/editors/visible.rs index 67813074a2bd..af25681943d3 100644 --- a/crates/re_data_ui/src/editors/visible.rs +++ b/crates/re_data_ui/src/editors/visible.rs @@ -13,12 +13,12 @@ pub fn edit_visible( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, + component: Option<&LatestAtComponentResults>, instance: &Instance, ) { let visible = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) .unwrap_or_else(|| default_visible(ctx, query, db, entity_path)); let mut edit_visible = visible; diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 50afb05c5b56..1ac43592bc6c 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -60,8 +60,8 @@ pub fn view_property_ui( blueprint_db, &blueprint_path, &blueprint_path, - component_results.get_or_empty(*component_name), - component_name, + component_results.get(*component_name), + *component_name, &0.into(), ); }), diff --git a/crates/re_viewer/src/ui/override_ui.rs b/crates/re_viewer/src/ui/override_ui.rs index 227e41dd2bcf..afea6ef6750a 100644 --- a/crates/re_viewer/src/ui/override_ui.rs +++ b/crates/re_viewer/src/ui/override_ui.rs @@ -140,8 +140,8 @@ pub fn override_ui( ctx.recording(), entity_path_overridden, &overrides.individual_override_path, - &results, - component_name, + Some(&results), + *component_name, instance, ); } else { diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 000c6c400e28..7be96273c857 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -61,7 +61,7 @@ type ComponentEditCallback = Box< &EntityDb, &EntityPath, &EntityPath, - &LatestAtComponentResults, + Option<&LatestAtComponentResults>, &Instance, ) + Send + Sync, @@ -163,13 +163,13 @@ impl ComponentUiRegistry { db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: &LatestAtComponentResults, - component_name: &ComponentName, + component: Option<&LatestAtComponentResults>, + component_name: ComponentName, instance: &Instance, ) { re_tracing::profile_function!(component_name.full_name()); - if let Some((_, edit_callback)) = self.component_editors.get(component_name) { + if let Some((_, edit_callback)) = self.component_editors.get(&component_name) { (*edit_callback)( ctx, ui, @@ -183,6 +183,7 @@ impl ComponentUiRegistry { ); } else { // Even if we can't edit the component, it's still helpful to show what the value is. + let empty = LatestAtComponentResults::empty(); self.ui( ctx, ui, @@ -190,7 +191,7 @@ impl ComponentUiRegistry { query, db, entity_path, - component, + component.unwrap_or(&empty), instance, ); } From eb70416f264a7fe020e6a06146e39914477a4bc2 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 23 May 2024 16:58:50 +0200 Subject: [PATCH 4/9] allow resetting to the default blueprint & empty value separately --- crates/re_space_view/src/view_property_ui.rs | 32 ++++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 1ac43592bc6c..88b7ab7183b7 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -67,9 +67,35 @@ pub fn view_property_ui( }), ); - if let Some(tooltip) = field_info.map(|info| info.documentation) { - list_item_response.on_hover_text(tooltip); - } + let list_item_response = + if let Some(tooltip) = field_info.map(|info| info.documentation) { + list_item_response.on_hover_text(tooltip) + } else { + list_item_response + }; + + list_item_response.context_menu(|ui| { + if ui.button("Reset to default blueprint.") + .on_hover_text("Resets this property to the value in the default blueprint.\n +If no default blueprint was set or it didn't set any value for this field, this is the same as resetting to empty.") + .clicked() { + ctx.reset_blueprint_component_by_name(&blueprint_path, *component_name); + ui.close_menu(); + } + ui.add_enabled_ui(component_results.contains_non_empty(*component_name), |ui| { + if ui.button("Reset to empty.") + .on_hover_text("Resets this property to an unset value, meaning that a heuristically determined value will be used instead.\n +This has the same effect as not setting the value in the blueprint at all.") + .on_disabled_hover_text("The property is already unset.") + .clicked() { + ctx.save_empty_blueprint_component_by_name(&blueprint_path, *component_name); + ui.close_menu(); + } + }); + + // TODO(andreas): The next logical thing here is now to save it to the default blueprint! + // This should be fairly straight forward except that we need to make sure that a default blueprint exists in the first place. + }); } }; From 0cd16d03b20be55817bc468a04aafb7ebaaf21be Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 23 May 2024 17:17:45 +0200 Subject: [PATCH 5/9] fmt --- crates/re_viewport_blueprint/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/re_viewport_blueprint/src/lib.rs b/crates/re_viewport_blueprint/src/lib.rs index 7ccb76c787a0..4c3e40b3fbed 100644 --- a/crates/re_viewport_blueprint/src/lib.rs +++ b/crates/re_viewport_blueprint/src/lib.rs @@ -14,8 +14,8 @@ pub use space_view::SpaceViewBlueprint; pub use space_view_contents::SpaceViewContents; pub use tree_actions::TreeAction; pub use view_properties::{ - edit_blueprint_component, entity_path_for_view_property, - query_view_property, query_view_property_or_default, view_property, + edit_blueprint_component, entity_path_for_view_property, query_view_property, + query_view_property_or_default, view_property, }; pub use viewport_blueprint::ViewportBlueprint; From 2525b4accbad3ff7c822052428e36465de798884 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 27 May 2024 15:16:51 +0200 Subject: [PATCH 6/9] less verbose on_hover_text --- crates/re_space_view/src/view_property_ui.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 9c9bc5ff50fe..2e2d5ecda40b 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -43,7 +43,7 @@ pub fn view_property_ui( let display_name = field_info.map_or_else(|| component_name.short_name(), |info| info.display_name); - let list_item_response = list_item::ListItem::new(re_ui) + let mut list_item_response = list_item::ListItem::new(re_ui) .interactive(false) .show_flat( ui, @@ -67,12 +67,9 @@ pub fn view_property_ui( }), ); - let list_item_response = - if let Some(tooltip) = field_info.map(|info| info.documentation) { - list_item_response.on_hover_text(tooltip) - } else { - list_item_response - }; + if let Some(tooltip) = field_info.map(|info| info.documentation) { + list_item_response = list_item_response.on_hover_text(tooltip); + } list_item_response.context_menu(|ui| { if ui.button("Reset to default blueprint.") From 89d5af22abb8f2e4e210b05687684728cae26137 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 27 May 2024 16:14:46 +0200 Subject: [PATCH 7/9] Revert "fix warning on first edit in generic view property archetype ui" This reverts commit 17b8a79c2138ee1bf9283fdc783dcb3aba0ca4dd. --- crates/re_data_ui/src/editors/corner2d.rs | 4 +- crates/re_data_ui/src/editors/mod.rs | 56 +++++++++---------- crates/re_data_ui/src/editors/visible.rs | 4 +- crates/re_selection_panel/src/override_ui.rs | 4 +- crates/re_space_view/src/view_property_ui.rs | 4 +- .../src/component_ui_registry.rs | 11 ++-- 6 files changed, 40 insertions(+), 43 deletions(-) diff --git a/crates/re_data_ui/src/editors/corner2d.rs b/crates/re_data_ui/src/editors/corner2d.rs index de941b641ef6..e82af0786e36 100644 --- a/crates/re_data_ui/src/editors/corner2d.rs +++ b/crates/re_data_ui/src/editors/corner2d.rs @@ -13,12 +13,12 @@ pub fn edit_corner2d( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let corner = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_corner2d(ctx, query, db, entity_path)); let mut edit_corner = corner; diff --git a/crates/re_data_ui/src/editors/mod.rs b/crates/re_data_ui/src/editors/mod.rs index 8173bd07a632..4c86de8bd8f0 100644 --- a/crates/re_data_ui/src/editors/mod.rs +++ b/crates/re_data_ui/src/editors/mod.rs @@ -27,12 +27,12 @@ fn edit_color_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_color = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_color(ctx, query, db, entity_path)); let current_color = current_color.into(); @@ -70,12 +70,12 @@ fn edit_text_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_text(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -110,12 +110,12 @@ fn edit_name_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_name(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -151,12 +151,12 @@ fn edit_scatter_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_scatter = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_scatter(ctx, query, db, entity_path)); let current_scatter = current_scatter.0; @@ -200,12 +200,12 @@ fn edit_radius_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_radius = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_radius(ctx, query, db, entity_path)); let current_radius = current_radius.0; @@ -247,12 +247,12 @@ fn edit_marker_shape_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_marker = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_marker_shape(ctx, query, db, entity_path)); let mut edit_marker = current_marker; @@ -352,12 +352,12 @@ fn edit_stroke_width_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_stroke_width = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_stroke_width(ctx, query, db, entity_path)); let current_stroke_width = current_stroke_width.0; @@ -399,12 +399,12 @@ fn edit_marker_size_ui( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let current_marker_size = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_marker_size(ctx, query, db, entity_path)); let current_marker_size = current_marker_size.0; @@ -437,22 +437,20 @@ fn default_marker_size( // ---- -type EditCallback = fn( - &ViewerContext<'_>, - &mut egui::Ui, - UiLayout, - &LatestAtQuery, - &EntityDb, - &EntityPath, - &EntityPath, - Option<&LatestAtComponentResults>, - &Instance, -); - fn register_editor<'a, C>( registry: &mut re_viewer_context::ComponentUiRegistry, default: fn(&ViewerContext<'_>, &LatestAtQuery, &EntityDb, &EntityPath) -> C, - edit: EditCallback, + edit: fn( + &ViewerContext<'_>, + &mut egui::Ui, + UiLayout, + &LatestAtQuery, + &EntityDb, + &EntityPath, + &EntityPath, + &LatestAtComponentResults, + &Instance, + ), ) where C: Component + Loggable + 'static + Into<::std::borrow::Cow<'a, C>>, { diff --git a/crates/re_data_ui/src/editors/visible.rs b/crates/re_data_ui/src/editors/visible.rs index af25681943d3..67813074a2bd 100644 --- a/crates/re_data_ui/src/editors/visible.rs +++ b/crates/re_data_ui/src/editors/visible.rs @@ -13,12 +13,12 @@ pub fn edit_visible( db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, + component: &LatestAtComponentResults, instance: &Instance, ) { let visible = component // TODO(#5607): what should happen if the promise is still pending? - .and_then(|c| c.instance::(db.resolver(), instance.get() as _)) + .instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_visible(ctx, query, db, entity_path)); let mut edit_visible = visible; diff --git a/crates/re_selection_panel/src/override_ui.rs b/crates/re_selection_panel/src/override_ui.rs index 79b462bce5d7..cca26fcf6be5 100644 --- a/crates/re_selection_panel/src/override_ui.rs +++ b/crates/re_selection_panel/src/override_ui.rs @@ -139,8 +139,8 @@ pub fn override_ui( ctx.recording(), entity_path_overridden, &overrides.individual_override_path, - Some(&results), - *component_name, + &results, + component_name, instance, ); } else { diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 2e2d5ecda40b..61022dc751dd 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -60,8 +60,8 @@ pub fn view_property_ui( blueprint_db, &blueprint_path, &blueprint_path, - component_results.get(*component_name), - *component_name, + component_results.get_or_empty(*component_name), + component_name, &0.into(), ); }), diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 7be96273c857..000c6c400e28 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -61,7 +61,7 @@ type ComponentEditCallback = Box< &EntityDb, &EntityPath, &EntityPath, - Option<&LatestAtComponentResults>, + &LatestAtComponentResults, &Instance, ) + Send + Sync, @@ -163,13 +163,13 @@ impl ComponentUiRegistry { db: &EntityDb, entity_path: &EntityPath, override_path: &EntityPath, - component: Option<&LatestAtComponentResults>, - component_name: ComponentName, + component: &LatestAtComponentResults, + component_name: &ComponentName, instance: &Instance, ) { re_tracing::profile_function!(component_name.full_name()); - if let Some((_, edit_callback)) = self.component_editors.get(&component_name) { + if let Some((_, edit_callback)) = self.component_editors.get(component_name) { (*edit_callback)( ctx, ui, @@ -183,7 +183,6 @@ impl ComponentUiRegistry { ); } else { // Even if we can't edit the component, it's still helpful to show what the value is. - let empty = LatestAtComponentResults::empty(); self.ui( ctx, ui, @@ -191,7 +190,7 @@ impl ComponentUiRegistry { query, db, entity_path, - component.unwrap_or(&empty), + component, instance, ); } From c1729b574d9db61d6c3f7fa4137ff03d23125094 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 27 May 2024 16:31:26 +0200 Subject: [PATCH 8/9] more robust handling of warning avoiding on out of bounds component access --- crates/re_data_ui/src/editors/corner2d.rs | 2 +- crates/re_data_ui/src/editors/mod.rs | 16 ++++----- crates/re_data_ui/src/editors/visible.rs | 2 +- crates/re_query/src/latest_at/helpers.rs | 42 +++++++++++++++++++++++ 4 files changed, 52 insertions(+), 10 deletions(-) diff --git a/crates/re_data_ui/src/editors/corner2d.rs b/crates/re_data_ui/src/editors/corner2d.rs index e82af0786e36..2f2f49369564 100644 --- a/crates/re_data_ui/src/editors/corner2d.rs +++ b/crates/re_data_ui/src/editors/corner2d.rs @@ -18,7 +18,7 @@ pub fn edit_corner2d( ) { let corner = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_corner2d(ctx, query, db, entity_path)); let mut edit_corner = corner; diff --git a/crates/re_data_ui/src/editors/mod.rs b/crates/re_data_ui/src/editors/mod.rs index 4c86de8bd8f0..8c654879761e 100644 --- a/crates/re_data_ui/src/editors/mod.rs +++ b/crates/re_data_ui/src/editors/mod.rs @@ -32,7 +32,7 @@ fn edit_color_ui( ) { let current_color = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_color(ctx, query, db, entity_path)); let current_color = current_color.into(); @@ -75,7 +75,7 @@ fn edit_text_ui( ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_text(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -115,7 +115,7 @@ fn edit_name_ui( ) { let current_text = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_name(ctx, query, db, entity_path)); let current_text = current_text.to_string(); @@ -156,7 +156,7 @@ fn edit_scatter_ui( ) { let current_scatter = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_scatter(ctx, query, db, entity_path)); let current_scatter = current_scatter.0; @@ -205,7 +205,7 @@ fn edit_radius_ui( ) { let current_radius = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_radius(ctx, query, db, entity_path)); let current_radius = current_radius.0; @@ -252,7 +252,7 @@ fn edit_marker_shape_ui( ) { let current_marker = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_marker_shape(ctx, query, db, entity_path)); let mut edit_marker = current_marker; @@ -357,7 +357,7 @@ fn edit_stroke_width_ui( ) { let current_stroke_width = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_stroke_width(ctx, query, db, entity_path)); let current_stroke_width = current_stroke_width.0; @@ -404,7 +404,7 @@ fn edit_marker_size_ui( ) { let current_marker_size = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_marker_size(ctx, query, db, entity_path)); let current_marker_size = current_marker_size.0; diff --git a/crates/re_data_ui/src/editors/visible.rs b/crates/re_data_ui/src/editors/visible.rs index 67813074a2bd..a9527549a5e1 100644 --- a/crates/re_data_ui/src/editors/visible.rs +++ b/crates/re_data_ui/src/editors/visible.rs @@ -18,7 +18,7 @@ pub fn edit_visible( ) { let visible = component // TODO(#5607): what should happen if the promise is still pending? - .instance::(db.resolver(), instance.get() as _) + .try_instance::(db.resolver(), instance.get() as _) .unwrap_or_else(|| default_visible(ctx, query, db, entity_path)); let mut edit_visible = visible; diff --git a/crates/re_query/src/latest_at/helpers.rs b/crates/re_query/src/latest_at/helpers.rs index fb52a8ee08f0..21f7d6f96355 100644 --- a/crates/re_query/src/latest_at/helpers.rs +++ b/crates/re_query/src/latest_at/helpers.rs @@ -149,6 +149,48 @@ impl LatestAtComponentResults { } } + /// Returns the component data of the specified instance if there's any ready data for this index. + /// + /// Returns None both for pending promises and if the index is out of bounds. + /// Logs an error only in case of deserialization failure. + #[inline] + pub fn try_instance( + &self, + resolver: &PromiseResolver, + index: usize, + ) -> Option { + let component_name = C::name(); + match self.to_dense::(resolver).flatten() { + PromiseResult::Pending => { + None + } + + PromiseResult::Ready(data) => { + // TODO(#5259): Figure out if/how we'd like to integrate clamping semantics into the + // selection panel. + // + // For now, we simply always clamp, which is the closest to the legacy behavior that the UI + // expects. + let index = usize::min(index, data.len().saturating_sub(1)); + + if data.len() > index { + Some(data[index].clone()) + } else { + None + } + } + + PromiseResult::Error(err) => { + re_log::warn_once!( + + "Couldn't deserialize {component_name}: {}", + re_error::format_ref(&*err), + ); + None + } + } + } + /// Returns the component data of the specified instance. /// /// Logs a warning and returns `None` if the component is missing or cannot be deserialized, or From 283ad930c1a8601719657dd7eacfa0b58a3c944c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 27 May 2024 17:09:24 +0200 Subject: [PATCH 9/9] cargo fmt --- crates/re_query/src/latest_at/helpers.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/re_query/src/latest_at/helpers.rs b/crates/re_query/src/latest_at/helpers.rs index 21f7d6f96355..523895f90aa3 100644 --- a/crates/re_query/src/latest_at/helpers.rs +++ b/crates/re_query/src/latest_at/helpers.rs @@ -161,9 +161,7 @@ impl LatestAtComponentResults { ) -> Option { let component_name = C::name(); match self.to_dense::(resolver).flatten() { - PromiseResult::Pending => { - None - } + PromiseResult::Pending => None, PromiseResult::Ready(data) => { // TODO(#5259): Figure out if/how we'd like to integrate clamping semantics into the @@ -182,7 +180,6 @@ impl LatestAtComponentResults { PromiseResult::Error(err) => { re_log::warn_once!( - "Couldn't deserialize {component_name}: {}", re_error::format_ref(&*err), );