From 8d0e5fdeebf44df3169cece8f6b32a8858631b13 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 27 May 2024 19:15:56 +0200 Subject: [PATCH 01/24] Replace `initial_override_value` with new `TypedComponentFallbackProvider` & `ComponentFallbackProvider` --- crates/re_selection_panel/src/override_ui.rs | 29 +++++--- .../src/visualizer_system.rs | 2 + .../src/visualizer_system.rs | 6 +- .../src/visualizers/arrows2d.rs | 2 + .../src/visualizers/arrows3d.rs | 2 + .../src/visualizers/assets3d.rs | 2 + .../src/visualizers/boxes2d.rs | 2 + .../src/visualizers/boxes3d.rs | 2 + .../src/visualizers/cameras.rs | 2 + .../src/visualizers/images.rs | 2 + .../src/visualizers/lines2d.rs | 2 + .../src/visualizers/lines3d.rs | 2 + .../src/visualizers/meshes.rs | 2 + .../src/visualizers/points2d.rs | 2 + .../src/visualizers/points3d.rs | 2 + .../src/visualizers/transform3d_arrows.rs | 2 + .../src/visualizer_system.rs | 2 + .../src/visualizer_system.rs | 2 + .../src/visualizer_system.rs | 2 + .../src/line_visualizer_system.rs | 32 ++++----- .../src/point_visualizer_system.rs | 32 ++++----- .../src/component_fallback_provider.rs | 68 +++++++++++++++++++ crates/re_viewer_context/src/lib.rs | 5 ++ .../src/space_view/space_view_class.rs | 16 +++++ .../src/space_view/visualizer_system.rs | 25 +++---- .../color_coordinates_visualizer_system.rs | 7 +- 26 files changed, 189 insertions(+), 65 deletions(-) create mode 100644 crates/re_viewer_context/src/component_fallback_provider.rs diff --git a/crates/re_selection_panel/src/override_ui.rs b/crates/re_selection_panel/src/override_ui.rs index 7377eb651560..998103038018 100644 --- a/crates/re_selection_panel/src/override_ui.rs +++ b/crates/re_selection_panel/src/override_ui.rs @@ -4,11 +4,12 @@ use itertools::Itertools; use re_data_store::LatestAtQuery; use re_entity_db::{EntityDb, InstancePath}; -use re_log_types::{DataRow, RowId, StoreKind}; +use re_log_types::{DataCell, DataRow, RowId, StoreKind}; use re_types_core::{components::VisualizerOverrides, ComponentName}; use re_viewer_context::{ - DataResult, OverridePath, SpaceViewClassExt as _, SystemCommand, SystemCommandSender as _, - UiLayout, ViewSystemIdentifier, ViewerContext, + ComponentFallbackResult, DataResult, FallbackProviderContext, OverridePath, + SpaceViewClassExt as _, SystemCommand, SystemCommandSender as _, UiLayout, + ViewSystemIdentifier, ViewerContext, }; use re_viewport_blueprint::SpaceViewBlueprint; @@ -229,13 +230,21 @@ pub fn add_new_override( .and_then(|result| result.2[0].clone()) .or_else(|| { view_systems.get_by_identifier(*viz).ok().and_then(|sys| { - sys.initial_override_value( - ctx, - query, - db.store(), - &data_result.entity_path, - component, - ) + if let ComponentFallbackResult::Value(value) = sys + .fallback_value( + &FallbackProviderContext { + ctx, + entity_path: &data_result.entity_path, + archetype_name: None, + query, + }, + *component, + ) + { + Some(DataCell::from_arrow(*component, value)) + } else { + None + } }) }) .or_else(|| { diff --git a/crates/re_space_view_bar_chart/src/visualizer_system.rs b/crates/re_space_view_bar_chart/src/visualizer_system.rs index df862bde1991..afa0f37bd119 100644 --- a/crates/re_space_view_bar_chart/src/visualizer_system.rs +++ b/crates/re_space_view_bar_chart/src/visualizer_system.rs @@ -84,3 +84,5 @@ impl VisualizerSystem for BarChartVisualizerSystem { self } } + +re_viewer_context::impl_component_fallback_provider!(BarChartVisualizerSystem => []); diff --git a/crates/re_space_view_dataframe/src/visualizer_system.rs b/crates/re_space_view_dataframe/src/visualizer_system.rs index 3d98a5bcadd8..79f4a782098d 100644 --- a/crates/re_space_view_dataframe/src/visualizer_system.rs +++ b/crates/re_space_view_dataframe/src/visualizer_system.rs @@ -1,6 +1,6 @@ use re_viewer_context::{ - IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, ViewQuery, - ViewerContext, VisualizerQueryInfo, VisualizerSystem, + ComponentFallbackProvider, IdentifiedViewSystem, SpaceViewSystemExecutionError, + ViewContextCollection, ViewQuery, ViewerContext, VisualizerQueryInfo, VisualizerSystem, }; /// An empty system to accept all entities in the space view @@ -31,3 +31,5 @@ impl VisualizerSystem for EmptySystem { self } } + +impl ComponentFallbackProvider for EmptySystem {} diff --git a/crates/re_space_view_spatial/src/visualizers/arrows2d.rs b/crates/re_space_view_spatial/src/visualizers/arrows2d.rs index e614dafb02b6..3bf8d7f2f54f 100644 --- a/crates/re_space_view_spatial/src/visualizers/arrows2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/arrows2d.rs @@ -300,3 +300,5 @@ impl VisualizerSystem for Arrows2DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Arrows2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/arrows3d.rs b/crates/re_space_view_spatial/src/visualizers/arrows3d.rs index 92e24b29859f..9d2376ddbda1 100644 --- a/crates/re_space_view_spatial/src/visualizers/arrows3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/arrows3d.rs @@ -303,3 +303,5 @@ impl VisualizerSystem for Arrows3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Arrows3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/assets3d.rs b/crates/re_space_view_spatial/src/visualizers/assets3d.rs index 2024ff475f03..ad54ab457432 100644 --- a/crates/re_space_view_spatial/src/visualizers/assets3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/assets3d.rs @@ -204,3 +204,5 @@ impl VisualizerSystem for Asset3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Asset3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/boxes2d.rs b/crates/re_space_view_spatial/src/visualizers/boxes2d.rs index c4f75a77999b..aacbce6a38eb 100644 --- a/crates/re_space_view_spatial/src/visualizers/boxes2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/boxes2d.rs @@ -306,3 +306,5 @@ impl VisualizerSystem for Boxes2DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Boxes2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/boxes3d.rs b/crates/re_space_view_spatial/src/visualizers/boxes3d.rs index d09fb6d41ee1..f96a7e5c0db2 100644 --- a/crates/re_space_view_spatial/src/visualizers/boxes3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/boxes3d.rs @@ -301,3 +301,5 @@ impl VisualizerSystem for Boxes3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Boxes3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/cameras.rs b/crates/re_space_view_spatial/src/visualizers/cameras.rs index ea6e24d74312..2ec6d4dcf22a 100644 --- a/crates/re_space_view_spatial/src/visualizers/cameras.rs +++ b/crates/re_space_view_spatial/src/visualizers/cameras.rs @@ -254,3 +254,5 @@ impl VisualizerSystem for CamerasVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(CamerasVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/images.rs b/crates/re_space_view_spatial/src/visualizers/images.rs index 2ec8f9f0c505..a6a231638f45 100644 --- a/crates/re_space_view_spatial/src/visualizers/images.rs +++ b/crates/re_space_view_spatial/src/visualizers/images.rs @@ -917,3 +917,5 @@ impl ImageVisualizer { Ok(()) } } + +re_viewer_context::impl_component_fallback_provider!(ImageVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/lines2d.rs b/crates/re_space_view_spatial/src/visualizers/lines2d.rs index 82be395413c1..ebd130c5a796 100644 --- a/crates/re_space_view_spatial/src/visualizers/lines2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/lines2d.rs @@ -286,3 +286,5 @@ impl VisualizerSystem for Lines2DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Lines2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/lines3d.rs b/crates/re_space_view_spatial/src/visualizers/lines3d.rs index e079d707b1cb..ce55148078cd 100644 --- a/crates/re_space_view_spatial/src/visualizers/lines3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/lines3d.rs @@ -295,3 +295,5 @@ impl VisualizerSystem for Lines3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Lines3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/meshes.rs b/crates/re_space_view_spatial/src/visualizers/meshes.rs index 0f9db1ce0189..c16f86c274bd 100644 --- a/crates/re_space_view_spatial/src/visualizers/meshes.rs +++ b/crates/re_space_view_spatial/src/visualizers/meshes.rs @@ -262,3 +262,5 @@ impl VisualizerSystem for Mesh3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Mesh3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/points2d.rs b/crates/re_space_view_spatial/src/visualizers/points2d.rs index b2338f542931..528be35c9143 100644 --- a/crates/re_space_view_spatial/src/visualizers/points2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/points2d.rs @@ -301,3 +301,5 @@ impl VisualizerSystem for Points2DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Points2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/points3d.rs b/crates/re_space_view_spatial/src/visualizers/points3d.rs index d04d4bda951e..62f06c43ffa3 100644 --- a/crates/re_space_view_spatial/src/visualizers/points3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/points3d.rs @@ -292,3 +292,5 @@ impl VisualizerSystem for Points3DVisualizer { self } } + +re_viewer_context::impl_component_fallback_provider!(Points3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs b/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs index dad69fe7082a..0188f097a87b 100644 --- a/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs +++ b/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs @@ -162,3 +162,5 @@ pub fn add_axis_arrows( .flags(LineStripFlags::FLAG_CAP_END_TRIANGLE | LineStripFlags::FLAG_CAP_START_ROUND) .picking_instance_id(picking_instance_id); } + +re_viewer_context::impl_component_fallback_provider!(Transform3DArrowsVisualizer => []); diff --git a/crates/re_space_view_tensor/src/visualizer_system.rs b/crates/re_space_view_tensor/src/visualizer_system.rs index f1c2bc55cff9..b711f936261c 100644 --- a/crates/re_space_view_tensor/src/visualizer_system.rs +++ b/crates/re_space_view_tensor/src/visualizer_system.rs @@ -51,6 +51,8 @@ impl VisualizerSystem for TensorSystem { } } +re_viewer_context::impl_component_fallback_provider!(TensorSystem => []); + impl TensorSystem { fn load_tensor_entity( &mut self, diff --git a/crates/re_space_view_text_document/src/visualizer_system.rs b/crates/re_space_view_text_document/src/visualizer_system.rs index 057dc85b1aee..386cbfac7622 100644 --- a/crates/re_space_view_text_document/src/visualizer_system.rs +++ b/crates/re_space_view_text_document/src/visualizer_system.rs @@ -72,3 +72,5 @@ impl VisualizerSystem for TextDocumentSystem { self } } + +re_viewer_context::impl_component_fallback_provider!(TextDocumentSystem => []); diff --git a/crates/re_space_view_text_log/src/visualizer_system.rs b/crates/re_space_view_text_log/src/visualizer_system.rs index 139c6256f40f..48edcb163934 100644 --- a/crates/re_space_view_text_log/src/visualizer_system.rs +++ b/crates/re_space_view_text_log/src/visualizer_system.rs @@ -126,6 +126,8 @@ impl VisualizerSystem for TextLogSystem { } } +re_viewer_context::impl_component_fallback_provider!(TextLogSystem => []); + // TODO(#5607): what should happen if the promise is still pending? #[inline] fn check_range<'a, C: Component>(results: &'a RangeData<'a, C>) -> re_query::Result<()> { diff --git a/crates/re_space_view_time_series/src/line_visualizer_system.rs b/crates/re_space_view_time_series/src/line_visualizer_system.rs index dcaeae4f9052..37531e23ee43 100644 --- a/crates/re_space_view_time_series/src/line_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/line_visualizer_system.rs @@ -7,8 +7,9 @@ use re_types::{ Archetype as _, ComponentNameSet, Loggable, }; use re_viewer_context::{ - AnnotationMap, DefaultColor, IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewQuery, - ViewerContext, VisualizerQueryInfo, VisualizerSystem, + AnnotationMap, DefaultColor, FallbackProviderContext, IdentifiedViewSystem, + SpaceViewSystemExecutionError, TypedComponentFallbackProvider, ViewQuery, ViewerContext, + VisualizerQueryInfo, VisualizerSystem, }; use crate::overrides::initial_override_color; @@ -69,25 +70,22 @@ impl VisualizerSystem for SeriesLineSystem { fn as_any(&self) -> &dyn std::any::Any { self } +} - fn initial_override_value( - &self, - _ctx: &ViewerContext<'_>, - _query: &re_data_store::LatestAtQuery, - _store: &re_data_store::DataStore, - entity_path: &re_log_types::EntityPath, - component: &re_types::ComponentName, - ) -> Option { - if *component == Color::name() { - Some([initial_override_color(entity_path)].into()) - } else if *component == StrokeWidth::name() { - Some([StrokeWidth(DEFAULT_STROKE_WIDTH)].into()) - } else { - None - } +impl TypedComponentFallbackProvider for SeriesLineSystem { + fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> Color { + initial_override_color(ctx.entity_path) } } +impl TypedComponentFallbackProvider for SeriesLineSystem { + fn fallback_value(&self, _ctx: &FallbackProviderContext<'_>) -> StrokeWidth { + StrokeWidth(DEFAULT_STROKE_WIDTH) + } +} + +re_viewer_context::impl_component_fallback_provider!(SeriesLineSystem => [Color, StrokeWidth]); + impl SeriesLineSystem { fn load_scalars( &mut self, diff --git a/crates/re_space_view_time_series/src/point_visualizer_system.rs b/crates/re_space_view_time_series/src/point_visualizer_system.rs index 04a0dd014218..159d7294172c 100644 --- a/crates/re_space_view_time_series/src/point_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/point_visualizer_system.rs @@ -7,8 +7,9 @@ use re_types::{ Archetype as _, ComponentNameSet, Loggable, }; use re_viewer_context::{ - AnnotationMap, DefaultColor, IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewQuery, - ViewerContext, VisualizerQueryInfo, VisualizerSystem, + AnnotationMap, DefaultColor, FallbackProviderContext, IdentifiedViewSystem, + SpaceViewSystemExecutionError, TypedComponentFallbackProvider, ViewQuery, ViewerContext, + VisualizerQueryInfo, VisualizerSystem, }; use crate::overrides::initial_override_color; @@ -72,25 +73,22 @@ impl VisualizerSystem for SeriesPointSystem { fn as_any(&self) -> &dyn std::any::Any { self } +} - fn initial_override_value( - &self, - _ctx: &ViewerContext<'_>, - _query: &re_data_store::LatestAtQuery, - _store: &re_data_store::DataStore, - entity_path: &re_log_types::EntityPath, - component: &re_types::ComponentName, - ) -> Option { - if *component == Color::name() { - Some([initial_override_color(entity_path)].into()) - } else if *component == MarkerSize::name() { - Some([MarkerSize(DEFAULT_MARKER_SIZE)].into()) - } else { - None - } +impl TypedComponentFallbackProvider for SeriesPointSystem { + fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> Color { + initial_override_color(ctx.entity_path) + } +} + +impl TypedComponentFallbackProvider for SeriesPointSystem { + fn fallback_value(&self, _ctx: &FallbackProviderContext<'_>) -> MarkerSize { + MarkerSize(DEFAULT_MARKER_SIZE) } } +re_viewer_context::impl_component_fallback_provider!(SeriesPointSystem => [Color, MarkerSize]); + impl SeriesPointSystem { fn load_scalars( &mut self, diff --git a/crates/re_viewer_context/src/component_fallback_provider.rs b/crates/re_viewer_context/src/component_fallback_provider.rs new file mode 100644 index 000000000000..fc1bf176475e --- /dev/null +++ b/crates/re_viewer_context/src/component_fallback_provider.rs @@ -0,0 +1,68 @@ +use re_types::external::arrow2; + +use crate::ViewerContext; + +// TODO: Docs +pub enum ComponentFallbackResult { + Value(Box), + SerializationError(re_types::SerializationError), + UnknownComponent, +} + +impl From for ComponentFallbackResult { + fn from(batch: T) -> Self { + match batch.to_arrow() { + Ok(value) => Self::Value(value), + Err(err) => Self::SerializationError(err), + } + } +} + +// TODO: Docs +pub struct FallbackProviderContext<'a> { + pub ctx: &'a ViewerContext<'a>, + pub entity_path: &'a re_log_types::EntityPath, + pub archetype_name: Option, + // TODO(andreas): Can we make this a `ViewQuery` instead? + // pub query: &'a ViewQuery<'a>, + pub query: &'a re_data_store::LatestAtQuery, + // pub view_state: &'a dyn SpaceViewState, // TODO(andreas): Need this, but don't know yet how to patch through everywhere. +} + +// TODO: Docs +pub trait ComponentFallbackProvider { + // TODO: Docs + fn fallback_value( + &self, + _ctx: &FallbackProviderContext<'_>, + _component: re_types::ComponentName, + ) -> ComponentFallbackResult { + ComponentFallbackResult::UnknownComponent + } +} + +// TODO: Docs +pub trait TypedComponentFallbackProvider { + fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> C; +} + +/// Implements the [`ComponentFallbackProvider`] trait for a given type, using a number of [`TypedComponentFallbackProvider`]. +#[macro_export] +macro_rules! impl_component_fallback_provider { + ($type:ty => [$($component:ty),*]) => { + impl re_viewer_context::ComponentFallbackProvider for $type { + fn fallback_value( + &self, + ctx: &re_viewer_context::FallbackProviderContext<'_>, + component_name: re_types::ComponentName, + ) -> re_viewer_context::ComponentFallbackResult { + $( + if component_name == <$component as re_types::Loggable>::name() { + return re_viewer_context::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); + } + )* + re_viewer_context::ComponentFallbackResult::UnknownComponent + } + } + }; +} diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 04edd44405db..fdee4671ac51 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -9,6 +9,7 @@ mod blueprint_id; mod caches; mod collapsed_id; mod command_sender; +mod component_fallback_provider; mod component_ui_registry; mod contents; mod item; @@ -41,6 +42,10 @@ pub use collapsed_id::{CollapseItem, CollapseScope, CollapsedId}; pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; +pub use component_fallback_provider::{ + ComponentFallbackProvider, ComponentFallbackResult, FallbackProviderContext, + TypedComponentFallbackProvider, +}; pub use component_ui_registry::{ComponentUiRegistry, UiLayout}; pub use contents::{blueprint_id_to_tile_id, Contents, ContentsName}; pub use item::Item; diff --git a/crates/re_viewer_context/src/space_view/space_view_class.rs b/crates/re_viewer_context/src/space_view/space_view_class.rs index 3e8256680654..570d804438a4 100644 --- a/crates/re_viewer_context/src/space_view/space_view_class.rs +++ b/crates/re_viewer_context/src/space_view/space_view_class.rs @@ -211,6 +211,22 @@ pub trait SpaceViewClass: Send + Sync { ) -> Result<(), SpaceViewSystemExecutionError>; } +// TODO: +// /// View classes are expected to be able to provide default values for any component of their property archetypes. +// impl ComponentFallbackProvider for dyn SpaceViewClass { +// fn fallback_value( +// &self, +// _ctx: &ViewerContext<'_>, +// _component: re_types::ComponentName, +// _archetype_name: Option, +// _entity: &re_log_types::EntityPath, +// _query: &ViewQuery<'_>, +// _view_state: &dyn SpaceViewState, +// ) -> Option> { +// None +// } +// } + pub trait SpaceViewClassExt<'a>: SpaceViewClass + 'a { /// Determines the set of visible entities for a given space view. // TODO(andreas): This should be part of the SpaceView's (non-blueprint) state. diff --git a/crates/re_viewer_context/src/space_view/visualizer_system.rs b/crates/re_viewer_context/src/space_view/visualizer_system.rs index e328c66d03e2..94ef0f6f21a6 100644 --- a/crates/re_viewer_context/src/space_view/visualizer_system.rs +++ b/crates/re_viewer_context/src/space_view/visualizer_system.rs @@ -3,9 +3,10 @@ use ahash::HashMap; use re_types::{Archetype, ComponentNameSet}; use crate::{ - ApplicableEntities, IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, - ViewQuery, ViewSystemIdentifier, ViewerContext, VisualizableEntities, - VisualizableFilterContext, VisualizerAdditionalApplicabilityFilter, + ApplicableEntities, ComponentFallbackProvider, IdentifiedViewSystem, + SpaceViewSystemExecutionError, ViewContextCollection, ViewQuery, ViewSystemIdentifier, + ViewerContext, VisualizableEntities, VisualizableFilterContext, + VisualizerAdditionalApplicabilityFilter, }; pub struct VisualizerQueryInfo { @@ -47,7 +48,10 @@ impl VisualizerQueryInfo { /// Element of a scene derived from a single archetype query. /// /// Is populated after scene contexts and has access to them. -pub trait VisualizerSystem: Send + Sync + 'static { +/// +/// All visualizers are expected to be able to provide a fallback value for any component they're using +/// via the [`ComponentFallbackProvider`] trait. +pub trait VisualizerSystem: Send + Sync + ComponentFallbackProvider + 'static { // TODO(andreas): This should be able to list out the ContextSystems it needs. /// Information about which components are queried by the visualizer. @@ -93,19 +97,6 @@ pub trait VisualizerSystem: Send + Sync + 'static { } fn as_any(&self) -> &dyn std::any::Any; - - /// Returns an initial value to use when creating an override for a component for this - /// visualizer. This is used as a fallback if the component doesn't already have data. - fn initial_override_value( - &self, - _ctx: &ViewerContext<'_>, - _query: &re_data_store::LatestAtQuery, - _store: &re_data_store::DataStore, - _entity_path: &re_log_types::EntityPath, - _component: &re_types::ComponentName, - ) -> Option { - None - } } pub struct VisualizerCollection { diff --git a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs index dbe217a2944e..01ab604b773b 100644 --- a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs +++ b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs @@ -4,8 +4,8 @@ use re_viewer::external::{ re_query, re_renderer, re_types::{self, components::Color, ComponentName, Loggable as _}, re_viewer_context::{ - IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, ViewQuery, - ViewSystemIdentifier, ViewerContext, VisualizerQueryInfo, VisualizerSystem, + self, IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, + ViewQuery, ViewSystemIdentifier, ViewerContext, VisualizerQueryInfo, VisualizerSystem, }, }; @@ -110,3 +110,6 @@ impl VisualizerSystem for InstanceColorSystem { self } } + +// TODO: document the role of this +re_viewer_context::impl_component_fallback_provider!(InstanceColorSystem => []); From b0e9846e83398a52d603fe79db67107438eccfc7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 28 May 2024 19:03:31 +0200 Subject: [PATCH 02/24] iterate on fallback provider --- crates/re_selection_panel/src/override_ui.rs | 6 ++-- .../src/component_fallback_provider.rs | 36 +++++++++++++------ crates/re_viewer_context/src/lib.rs | 2 +- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/crates/re_selection_panel/src/override_ui.rs b/crates/re_selection_panel/src/override_ui.rs index 998103038018..ef4fa942fb84 100644 --- a/crates/re_selection_panel/src/override_ui.rs +++ b/crates/re_selection_panel/src/override_ui.rs @@ -232,9 +232,9 @@ pub fn add_new_override( view_systems.get_by_identifier(*viz).ok().and_then(|sys| { if let ComponentFallbackResult::Value(value) = sys .fallback_value( - &FallbackProviderContext { - ctx, - entity_path: &data_result.entity_path, + &QueryContext { + viewer_ctx: ctx, + target_entity_path: &data_result.entity_path, archetype_name: None, query, }, diff --git a/crates/re_viewer_context/src/component_fallback_provider.rs b/crates/re_viewer_context/src/component_fallback_provider.rs index fc1bf176475e..3b7853722dbb 100644 --- a/crates/re_viewer_context/src/component_fallback_provider.rs +++ b/crates/re_viewer_context/src/component_fallback_provider.rs @@ -18,11 +18,25 @@ impl From for ComponentFallbackResult { } } -// TODO: Docs -pub struct FallbackProviderContext<'a> { - pub ctx: &'a ViewerContext<'a>, - pub entity_path: &'a re_log_types::EntityPath, +/// Context for a latest at query in a specific view. +/// +/// TODO: move elsewhere +/// TODO: this is centered around latest-at queries. does it have to be +pub struct QueryContext<'a> { + pub viewer_ctx: &'a ViewerContext<'a>, + + /// Target entity path which is lacking the component and needs a fallback. + /// + /// For editing overrides/defaults, this is the path to the store entity where they override/default is used. + /// For view properties this is the path that stores the respective view property archetype. + pub target_entity_path: &'a re_log_types::EntityPath, + + /// Archetype name in which context the component is needed. + /// + /// View properties always have an archetype context, but overrides/defaults may not. pub archetype_name: Option, + + /// Query which didn't yield a result for the component at the target entity path. // TODO(andreas): Can we make this a `ViewQuery` instead? // pub query: &'a ViewQuery<'a>, pub query: &'a re_data_store::LatestAtQuery, @@ -34,7 +48,7 @@ pub trait ComponentFallbackProvider { // TODO: Docs fn fallback_value( &self, - _ctx: &FallbackProviderContext<'_>, + _ctx: &QueryContext<'_>, _component: re_types::ComponentName, ) -> ComponentFallbackResult { ComponentFallbackResult::UnknownComponent @@ -43,25 +57,25 @@ pub trait ComponentFallbackProvider { // TODO: Docs pub trait TypedComponentFallbackProvider { - fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> C; + fn fallback_value(&self, ctx: &QueryContext<'_>) -> C; } /// Implements the [`ComponentFallbackProvider`] trait for a given type, using a number of [`TypedComponentFallbackProvider`]. #[macro_export] macro_rules! impl_component_fallback_provider { ($type:ty => [$($component:ty),*]) => { - impl re_viewer_context::ComponentFallbackProvider for $type { + impl $crate::ComponentFallbackProvider for $type { fn fallback_value( &self, - ctx: &re_viewer_context::FallbackProviderContext<'_>, + ctx: &$crate::QueryContext<'_>, component_name: re_types::ComponentName, - ) -> re_viewer_context::ComponentFallbackResult { + ) -> $crate::ComponentFallbackResult { $( if component_name == <$component as re_types::Loggable>::name() { - return re_viewer_context::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); + return $crate::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); } )* - re_viewer_context::ComponentFallbackResult::UnknownComponent + $crate::ComponentFallbackResult::UnknownComponent } } }; diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index fdee4671ac51..017a399e94d4 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -43,7 +43,7 @@ pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; pub use component_fallback_provider::{ - ComponentFallbackProvider, ComponentFallbackResult, FallbackProviderContext, + ComponentFallbackProvider, ComponentFallbackResult, QueryContext, TypedComponentFallbackProvider, }; pub use component_ui_registry::{ComponentUiRegistry, UiLayout}; From 8133695c61fc54b81f305d51d64b4710c563f85f Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 29 May 2024 11:16:59 +0200 Subject: [PATCH 03/24] all visualizers have to implement `as_fallback_provider` conversion function --- crates/re_space_view_bar_chart/src/visualizer_system.rs | 4 ++++ crates/re_space_view_dataframe/src/visualizer_system.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/arrows2d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/arrows3d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/assets3d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/boxes2d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/boxes3d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/cameras.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/images.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/lines2d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/lines3d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/meshes.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/points2d.rs | 4 ++++ crates/re_space_view_spatial/src/visualizers/points3d.rs | 4 ++++ .../src/visualizers/transform3d_arrows.rs | 4 ++++ crates/re_space_view_tensor/src/visualizer_system.rs | 4 ++++ crates/re_space_view_text_document/src/visualizer_system.rs | 4 ++++ crates/re_space_view_text_log/src/visualizer_system.rs | 4 ++++ .../re_space_view_time_series/src/line_visualizer_system.rs | 4 ++++ .../src/point_visualizer_system.rs | 4 ++++ .../re_viewer_context/src/space_view/visualizer_system.rs | 6 ++++++ .../src/color_coordinates_visualizer_system.rs | 4 ++++ 22 files changed, 90 insertions(+) diff --git a/crates/re_space_view_bar_chart/src/visualizer_system.rs b/crates/re_space_view_bar_chart/src/visualizer_system.rs index afa0f37bd119..f8cf9f7fae1e 100644 --- a/crates/re_space_view_bar_chart/src/visualizer_system.rs +++ b/crates/re_space_view_bar_chart/src/visualizer_system.rs @@ -83,6 +83,10 @@ impl VisualizerSystem for BarChartVisualizerSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(BarChartVisualizerSystem => []); diff --git a/crates/re_space_view_dataframe/src/visualizer_system.rs b/crates/re_space_view_dataframe/src/visualizer_system.rs index 79f4a782098d..b316d5c07743 100644 --- a/crates/re_space_view_dataframe/src/visualizer_system.rs +++ b/crates/re_space_view_dataframe/src/visualizer_system.rs @@ -30,6 +30,10 @@ impl VisualizerSystem for EmptySystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } impl ComponentFallbackProvider for EmptySystem {} diff --git a/crates/re_space_view_spatial/src/visualizers/arrows2d.rs b/crates/re_space_view_spatial/src/visualizers/arrows2d.rs index 3bf8d7f2f54f..af28b71d7cc6 100644 --- a/crates/re_space_view_spatial/src/visualizers/arrows2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/arrows2d.rs @@ -299,6 +299,10 @@ impl VisualizerSystem for Arrows2DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Arrows2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/arrows3d.rs b/crates/re_space_view_spatial/src/visualizers/arrows3d.rs index 9d2376ddbda1..7210cd267af1 100644 --- a/crates/re_space_view_spatial/src/visualizers/arrows3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/arrows3d.rs @@ -302,6 +302,10 @@ impl VisualizerSystem for Arrows3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Arrows3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/assets3d.rs b/crates/re_space_view_spatial/src/visualizers/assets3d.rs index ad54ab457432..d487188a557b 100644 --- a/crates/re_space_view_spatial/src/visualizers/assets3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/assets3d.rs @@ -203,6 +203,10 @@ impl VisualizerSystem for Asset3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Asset3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/boxes2d.rs b/crates/re_space_view_spatial/src/visualizers/boxes2d.rs index aacbce6a38eb..f53daa226841 100644 --- a/crates/re_space_view_spatial/src/visualizers/boxes2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/boxes2d.rs @@ -305,6 +305,10 @@ impl VisualizerSystem for Boxes2DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Boxes2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/boxes3d.rs b/crates/re_space_view_spatial/src/visualizers/boxes3d.rs index f96a7e5c0db2..56d92323b400 100644 --- a/crates/re_space_view_spatial/src/visualizers/boxes3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/boxes3d.rs @@ -300,6 +300,10 @@ impl VisualizerSystem for Boxes3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Boxes3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/cameras.rs b/crates/re_space_view_spatial/src/visualizers/cameras.rs index 2ec6d4dcf22a..4357b251ce40 100644 --- a/crates/re_space_view_spatial/src/visualizers/cameras.rs +++ b/crates/re_space_view_spatial/src/visualizers/cameras.rs @@ -253,6 +253,10 @@ impl VisualizerSystem for CamerasVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(CamerasVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/images.rs b/crates/re_space_view_spatial/src/visualizers/images.rs index a6a231638f45..fa7c30f32eee 100644 --- a/crates/re_space_view_spatial/src/visualizers/images.rs +++ b/crates/re_space_view_spatial/src/visualizers/images.rs @@ -840,6 +840,10 @@ impl VisualizerSystem for ImageVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } impl ImageVisualizer { diff --git a/crates/re_space_view_spatial/src/visualizers/lines2d.rs b/crates/re_space_view_spatial/src/visualizers/lines2d.rs index ebd130c5a796..a1cdbf19b724 100644 --- a/crates/re_space_view_spatial/src/visualizers/lines2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/lines2d.rs @@ -285,6 +285,10 @@ impl VisualizerSystem for Lines2DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Lines2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/lines3d.rs b/crates/re_space_view_spatial/src/visualizers/lines3d.rs index ce55148078cd..0cdbe625257c 100644 --- a/crates/re_space_view_spatial/src/visualizers/lines3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/lines3d.rs @@ -294,6 +294,10 @@ impl VisualizerSystem for Lines3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Lines3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/meshes.rs b/crates/re_space_view_spatial/src/visualizers/meshes.rs index c16f86c274bd..7889165aeccc 100644 --- a/crates/re_space_view_spatial/src/visualizers/meshes.rs +++ b/crates/re_space_view_spatial/src/visualizers/meshes.rs @@ -261,6 +261,10 @@ impl VisualizerSystem for Mesh3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Mesh3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/points2d.rs b/crates/re_space_view_spatial/src/visualizers/points2d.rs index 528be35c9143..0e2da7b12614 100644 --- a/crates/re_space_view_spatial/src/visualizers/points2d.rs +++ b/crates/re_space_view_spatial/src/visualizers/points2d.rs @@ -300,6 +300,10 @@ impl VisualizerSystem for Points2DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Points2DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/points3d.rs b/crates/re_space_view_spatial/src/visualizers/points3d.rs index 62f06c43ffa3..cb086e0555fc 100644 --- a/crates/re_space_view_spatial/src/visualizers/points3d.rs +++ b/crates/re_space_view_spatial/src/visualizers/points3d.rs @@ -291,6 +291,10 @@ impl VisualizerSystem for Points3DVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(Points3DVisualizer => []); diff --git a/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs b/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs index 0188f097a87b..c4e4f92f54cb 100644 --- a/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs +++ b/crates/re_space_view_spatial/src/visualizers/transform3d_arrows.rs @@ -113,6 +113,10 @@ impl VisualizerSystem for Transform3DArrowsVisualizer { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } const AXIS_COLOR_X: Color32 = Color32::from_rgb(255, 25, 25); diff --git a/crates/re_space_view_tensor/src/visualizer_system.rs b/crates/re_space_view_tensor/src/visualizer_system.rs index b711f936261c..7c12d3c79948 100644 --- a/crates/re_space_view_tensor/src/visualizer_system.rs +++ b/crates/re_space_view_tensor/src/visualizer_system.rs @@ -49,6 +49,10 @@ impl VisualizerSystem for TensorSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(TensorSystem => []); diff --git a/crates/re_space_view_text_document/src/visualizer_system.rs b/crates/re_space_view_text_document/src/visualizer_system.rs index 386cbfac7622..3a849f62efa5 100644 --- a/crates/re_space_view_text_document/src/visualizer_system.rs +++ b/crates/re_space_view_text_document/src/visualizer_system.rs @@ -71,6 +71,10 @@ impl VisualizerSystem for TextDocumentSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(TextDocumentSystem => []); diff --git a/crates/re_space_view_text_log/src/visualizer_system.rs b/crates/re_space_view_text_log/src/visualizer_system.rs index 48edcb163934..bb138f8d3486 100644 --- a/crates/re_space_view_text_log/src/visualizer_system.rs +++ b/crates/re_space_view_text_log/src/visualizer_system.rs @@ -124,6 +124,10 @@ impl VisualizerSystem for TextLogSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } re_viewer_context::impl_component_fallback_provider!(TextLogSystem => []); diff --git a/crates/re_space_view_time_series/src/line_visualizer_system.rs b/crates/re_space_view_time_series/src/line_visualizer_system.rs index 37531e23ee43..b0d9457a2381 100644 --- a/crates/re_space_view_time_series/src/line_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/line_visualizer_system.rs @@ -70,6 +70,10 @@ impl VisualizerSystem for SeriesLineSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } impl TypedComponentFallbackProvider for SeriesLineSystem { diff --git a/crates/re_space_view_time_series/src/point_visualizer_system.rs b/crates/re_space_view_time_series/src/point_visualizer_system.rs index 159d7294172c..81087a2a3921 100644 --- a/crates/re_space_view_time_series/src/point_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/point_visualizer_system.rs @@ -73,6 +73,10 @@ impl VisualizerSystem for SeriesPointSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } impl TypedComponentFallbackProvider for SeriesPointSystem { diff --git a/crates/re_viewer_context/src/space_view/visualizer_system.rs b/crates/re_viewer_context/src/space_view/visualizer_system.rs index 94ef0f6f21a6..62e3baa4c0ff 100644 --- a/crates/re_viewer_context/src/space_view/visualizer_system.rs +++ b/crates/re_viewer_context/src/space_view/visualizer_system.rs @@ -97,6 +97,12 @@ pub trait VisualizerSystem: Send + Sync + ComponentFallbackProvider + 'static { } fn as_any(&self) -> &dyn std::any::Any; + + /// Casts to a fallback provider. + /// + /// This is the same workaround as `as_any`: + /// It's not possible to cast &dyn [`VisualizerSystem`] to &dyn [`ComponentFallbackProvider`] otherwise. + fn as_fallback_provider(&self) -> &dyn ComponentFallbackProvider; } pub struct VisualizerCollection { diff --git a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs index 01ab604b773b..89db4510ee9f 100644 --- a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs +++ b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs @@ -109,6 +109,10 @@ impl VisualizerSystem for InstanceColorSystem { fn as_any(&self) -> &dyn std::any::Any { self } + + fn as_fallback_provider(&self) -> &dyn re_viewer_context::ComponentFallbackProvider { + self + } } // TODO: document the role of this From f6dd969559f52382935bac767e6acafb1ebcfbf5 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 29 May 2024 11:50:23 +0200 Subject: [PATCH 04/24] query context rename leftover --- .../src/line_visualizer_system.rs | 12 ++++++------ .../src/point_visualizer_system.rs | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/re_space_view_time_series/src/line_visualizer_system.rs b/crates/re_space_view_time_series/src/line_visualizer_system.rs index b0d9457a2381..5087f5062b67 100644 --- a/crates/re_space_view_time_series/src/line_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/line_visualizer_system.rs @@ -7,9 +7,9 @@ use re_types::{ Archetype as _, ComponentNameSet, Loggable, }; use re_viewer_context::{ - AnnotationMap, DefaultColor, FallbackProviderContext, IdentifiedViewSystem, - SpaceViewSystemExecutionError, TypedComponentFallbackProvider, ViewQuery, ViewerContext, - VisualizerQueryInfo, VisualizerSystem, + AnnotationMap, DefaultColor, IdentifiedViewSystem, QueryContext, SpaceViewSystemExecutionError, + TypedComponentFallbackProvider, ViewQuery, ViewerContext, VisualizerQueryInfo, + VisualizerSystem, }; use crate::overrides::initial_override_color; @@ -77,13 +77,13 @@ impl VisualizerSystem for SeriesLineSystem { } impl TypedComponentFallbackProvider for SeriesLineSystem { - fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> Color { - initial_override_color(ctx.entity_path) + fn fallback_value(&self, ctx: &QueryContext<'_>) -> Color { + initial_override_color(ctx.target_entity_path) } } impl TypedComponentFallbackProvider for SeriesLineSystem { - fn fallback_value(&self, _ctx: &FallbackProviderContext<'_>) -> StrokeWidth { + fn fallback_value(&self, _ctx: &QueryContext<'_>) -> StrokeWidth { StrokeWidth(DEFAULT_STROKE_WIDTH) } } diff --git a/crates/re_space_view_time_series/src/point_visualizer_system.rs b/crates/re_space_view_time_series/src/point_visualizer_system.rs index 81087a2a3921..410d5ebdf08f 100644 --- a/crates/re_space_view_time_series/src/point_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/point_visualizer_system.rs @@ -7,9 +7,9 @@ use re_types::{ Archetype as _, ComponentNameSet, Loggable, }; use re_viewer_context::{ - AnnotationMap, DefaultColor, FallbackProviderContext, IdentifiedViewSystem, - SpaceViewSystemExecutionError, TypedComponentFallbackProvider, ViewQuery, ViewerContext, - VisualizerQueryInfo, VisualizerSystem, + AnnotationMap, DefaultColor, IdentifiedViewSystem, QueryContext, SpaceViewSystemExecutionError, + TypedComponentFallbackProvider, ViewQuery, ViewerContext, VisualizerQueryInfo, + VisualizerSystem, }; use crate::overrides::initial_override_color; @@ -80,13 +80,13 @@ impl VisualizerSystem for SeriesPointSystem { } impl TypedComponentFallbackProvider for SeriesPointSystem { - fn fallback_value(&self, ctx: &FallbackProviderContext<'_>) -> Color { - initial_override_color(ctx.entity_path) + fn fallback_value(&self, ctx: &QueryContext<'_>) -> Color { + initial_override_color(ctx.target_entity_path) } } impl TypedComponentFallbackProvider for SeriesPointSystem { - fn fallback_value(&self, _ctx: &FallbackProviderContext<'_>) -> MarkerSize { + fn fallback_value(&self, _ctx: &QueryContext<'_>) -> MarkerSize { MarkerSize(DEFAULT_MARKER_SIZE) } } From 2e51ad5b75c4bf66edad4125926ebf8181e29364 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 29 May 2024 12:02:48 +0200 Subject: [PATCH 05/24] use new fallback provider with edit ui and introduce typed edit ui --- Cargo.lock | 1 + crates/re_edit_ui/src/corner2d.rs | 70 +-- crates/re_edit_ui/src/lib.rs | 479 +++--------------- crates/re_edit_ui/src/marker_shape.rs | 85 ++++ crates/re_edit_ui/src/visible.rs | 43 +- crates/re_selection_panel/src/override_ui.rs | 88 ++-- crates/re_space_view/src/view_property_ui.rs | 19 +- .../src/space_view_class.rs | 17 +- crates/re_viewer_context/Cargo.toml | 1 + .../src/component_ui_registry.rs | 228 ++++++--- 10 files changed, 404 insertions(+), 627 deletions(-) create mode 100644 crates/re_edit_ui/src/marker_shape.rs diff --git a/Cargo.lock b/Cargo.lock index 415787767f83..e4f259bc6b6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5223,6 +5223,7 @@ dependencies = [ "re_data_source", "re_data_store", "re_entity_db", + "re_error", "re_log", "re_log_types", "re_query", diff --git a/crates/re_edit_ui/src/corner2d.rs b/crates/re_edit_ui/src/corner2d.rs index 6ea8bf62453e..7d91bfebef5f 100644 --- a/crates/re_edit_ui/src/corner2d.rs +++ b/crates/re_edit_ui/src/corner2d.rs @@ -1,69 +1,35 @@ -use re_data_store::LatestAtQuery; -use re_entity_db::{external::re_query::LatestAtComponentResults, EntityDb}; -use re_log_types::{EntityPath, Instance}; use re_types::blueprint::components::Corner2D; -use re_viewer_context::{UiLayout, ViewerContext}; +use re_viewer_context::ViewerContext; -#[allow(clippy::too_many_arguments)] pub fn edit_corner2d( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let corner = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_corner2d(ctx, query, db, entity_path)); - let mut edit_corner = corner; - - egui::ComboBox::from_id_source("corner2d") - .selected_text(format!("{corner}")) + value: &mut Corner2D, +) -> egui::Response { + let outer_response = egui::ComboBox::from_id_source("corner2d") + .selected_text(format!("{value}")) .show_ui(ui, |ui| { ui.selectable_value( - &mut edit_corner, + value, egui_plot::Corner::LeftTop.into(), format!("{}", Corner2D::from(egui_plot::Corner::LeftTop)), - ); - ui.selectable_value( - &mut edit_corner, + ) + .union(ui.selectable_value( + value, egui_plot::Corner::RightTop.into(), format!("{}", Corner2D::from(egui_plot::Corner::RightTop)), - ); - ui.selectable_value( - &mut edit_corner, + )) + .union(ui.selectable_value( + value, egui_plot::Corner::LeftBottom.into(), format!("{}", Corner2D::from(egui_plot::Corner::LeftBottom)), - ); - ui.selectable_value( - &mut edit_corner, + )) + .union(ui.selectable_value( + value, egui_plot::Corner::RightBottom.into(), format!("{}", Corner2D::from(egui_plot::Corner::RightBottom)), - ); + )) }); - if corner != edit_corner { - ctx.save_blueprint_component(override_path, &edit_corner); - } -} - -#[inline] -pub fn default_corner2d( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> Corner2D { - // TODO(#4194): Want to distinguish the space view this happens in. - // TimeSeriesView: RightBottom - // BarChart: RightTop - // Need to make handling of editors a bit more powerful for this. - // Rough idea right now is to have "default providers" which can be either a View or a Visualizer. - // They then get queried with a ComponentName and return LatestAtComponentResults (or similar). - Corner2D::RightBottom + outer_response.inner.unwrap_or(outer_response.response) } diff --git a/crates/re_edit_ui/src/lib.rs b/crates/re_edit_ui/src/lib.rs index 61ba6339edaa..3a383c03309c 100644 --- a/crates/re_edit_ui/src/lib.rs +++ b/crates/re_edit_ui/src/lib.rs @@ -3,481 +3,116 @@ //! The only entry point is [`register_editors`], which registers all editors in the component UI registry. //! This should be called by `re_viewer` on startup. -// TODO(jleibs): Turn these methods into a trait. - mod corner2d; +mod marker_shape; mod visible; use egui::NumExt as _; -use re_data_store::LatestAtQuery; -use re_entity_db::{external::re_query::LatestAtComponentResults, EntityDb}; -use re_log_types::{EntityPath, Instance}; -use re_types::{ - blueprint::components::{Corner2D, Visible}, - components::{ - Color, MarkerShape, MarkerSize, Name, Radius, ScalarScattering, StrokeWidth, Text, - }, - Component, Loggable, -}; -use re_viewer_context::{UiLayout, ViewerContext}; +use re_types::components::{Color, MarkerSize, Name, Radius, ScalarScattering, StrokeWidth, Text}; +use re_viewer_context::ViewerContext; // ---- -#[allow(clippy::too_many_arguments)] -fn edit_color_ui( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_color = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_color(ctx, query, db, entity_path)); - - let current_color = current_color.into(); - let mut edit_color = current_color; - - egui::color_picker::color_edit_button_srgba( +fn edit_color_ui(_ctx: &ViewerContext<'_>, ui: &mut egui::Ui, value: &mut Color) -> egui::Response { + let mut edit_color = (*value).into(); + let response = egui::color_picker::color_edit_button_srgba( ui, &mut edit_color, + // TODO(#1611): No transparency supported right now. + // Once we do we probably need to be more explicit about the component semantics. egui::color_picker::Alpha::Opaque, ); - - if edit_color != current_color { - ctx.save_blueprint_component(override_path, &Color::from(edit_color)); - } + *value = edit_color.into(); + response } -#[inline] -fn default_color( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> Color { - Color::from_rgb(255, 255, 255) +fn edit_text_ui(_ctx: &ViewerContext<'_>, ui: &mut egui::Ui, value: &mut Text) -> egui::Response { + let mut edit_text = value.to_string(); + let response = egui::TextEdit::singleline(&mut edit_text).show(ui).response; + *value = edit_text.into(); + response } -// ---- - -#[allow(clippy::too_many_arguments)] -fn edit_text_ui( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_text = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_text(ctx, query, db, entity_path)); - - let current_text = current_text.to_string(); - let mut edit_text = current_text.clone(); - - egui::TextEdit::singleline(&mut edit_text).show(ui); - - if edit_text != current_text { - let new_text = Text::from(edit_text); - - ctx.save_blueprint_component(override_path, &new_text); - } +fn edit_name_ui(_ctx: &ViewerContext<'_>, ui: &mut egui::Ui, value: &mut Name) -> egui::Response { + let mut edit_name = value.to_string(); + let response = egui::TextEdit::singleline(&mut edit_name).show(ui).response; + *value = edit_name.into(); + response } -#[inline] -fn default_text( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - entity_path: &EntityPath, -) -> Text { - Text::from(entity_path.to_string()) -} - -// ---- -#[allow(clippy::too_many_arguments)] -fn edit_name_ui( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_text = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_name(ctx, query, db, entity_path)); - - let current_text = current_text.to_string(); - let mut edit_text = current_text.clone(); - - egui::TextEdit::singleline(&mut edit_text).show(ui); - - if edit_text != current_text { - let new_text = Name::from(edit_text); - - ctx.save_blueprint_component(override_path, &new_text); - } -} - -#[inline] -fn default_name( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - entity_path: &EntityPath, -) -> Name { - Name::from(entity_path.to_string()) -} - -// ---- - -#[allow(clippy::too_many_arguments)] fn edit_scatter_ui( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_scatter = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_scatter(ctx, query, db, entity_path)); - - let current_scatter = current_scatter.0; - let mut edit_scatter = current_scatter; + value: &mut ScalarScattering, +) -> egui::Response { + let scattered_text = if value.0 { "Scattered" } else { "Line" }; - let scattered_text = if current_scatter { "Scattered" } else { "Line" }; - - egui::ComboBox::from_id_source("scatter") + let outer_response = egui::ComboBox::from_id_source("scatter") .selected_text(scattered_text) .show_ui(ui, |ui| { - ui.selectable_value(&mut edit_scatter, false, "Line"); - ui.selectable_value(&mut edit_scatter, true, "Scattered"); + ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); + ui.selectable_value(&mut value.0, false, "Line") + .union(ui.selectable_value(&mut value.0, true, "Scattered")) }); - if edit_scatter != current_scatter { - let new_scatter = ScalarScattering::from(edit_scatter); - - ctx.save_blueprint_component(override_path, &new_scatter); - } -} - -#[inline] -fn default_scatter( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> ScalarScattering { - ScalarScattering::from(false) + outer_response.inner.unwrap_or(outer_response.response) } -// ---- - -#[allow(clippy::too_many_arguments)] fn edit_radius_ui( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_radius = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_radius(ctx, query, db, entity_path)); - - let current_radius = current_radius.0; - let mut edit_radius = current_radius; - - let speed = (current_radius * 0.01).at_least(0.001); + value: &mut Radius, +) -> egui::Response { + let speed = (value.0 * 0.01).at_least(0.001); ui.add( - egui::DragValue::new(&mut edit_radius) + egui::DragValue::new(&mut value.0) .clamp_range(0.0..=f64::INFINITY) .speed(speed), - ); - - if edit_radius != current_radius { - let new_radius = Radius::from(edit_radius); - - ctx.save_blueprint_component(override_path, &new_radius); - } -} - -#[inline] -fn default_radius( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> Radius { - Radius::from(1.0) -} - -// ---- - -#[allow(clippy::too_many_arguments)] -fn edit_marker_shape_ui( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_marker = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_marker_shape(ctx, query, db, entity_path)); - - let mut edit_marker = current_marker; - - let marker_text = edit_marker.to_string(); - - let item_width = 100.0; - - egui::ComboBox::from_id_source("marker_shape") - .selected_text(marker_text) // TODO(emilk): Show marker shape in the selected text - .width( - ui.available_width() - .at_most(item_width + ui.spacing().menu_margin.sum().x), - ) - .height(320.0) - .show_ui(ui, |ui| { - let background_x_range = (ui.max_rect() + ui.spacing().menu_margin).x_range(); - - let list_ui = |ui: &mut egui::Ui| { - for marker in MarkerShape::ALL { - let response = ctx - .re_ui - .list_item() - .selected(edit_marker == marker) - .show_flat( - ui, - re_ui::list_item::LabelContent::new(marker.to_string()) - .min_desired_width(item_width) - .with_icon_fn(|_re_ui, ui, rect, visuals| { - paint_marker(ui, marker.into(), rect, visuals.text_color()); - }), - ); - - if response.clicked() { - edit_marker = marker; - } - } - }; - - re_ui::full_span::full_span_scope(ui, background_x_range, |ui| { - re_ui::list_item::list_item_scope(ui, "marker_shape", list_ui); - }); - }); - - if edit_marker != current_marker { - ctx.save_blueprint_component(override_path, &edit_marker); - } + ) } -#[inline] -fn default_marker_shape( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> MarkerShape { - MarkerShape::default() -} - -fn paint_marker( - ui: &egui::Ui, - marker: egui_plot::MarkerShape, - rect: egui::Rect, - color: egui::Color32, -) { - use egui_plot::PlotItem as _; - - let points = egui_plot::Points::new([0.0, 0.0]) - .shape(marker) - .color(color) - .radius(rect.size().min_elem() / 2.0) - .filled(true); - - let bounds = egui_plot::PlotBounds::new_symmetrical(0.5); - let transform = egui_plot::PlotTransform::new(rect, bounds, true, true); - - let mut shapes = vec![]; - points.shapes(ui, &transform, &mut shapes); - ui.painter().extend(shapes); -} - -// ---- - -#[allow(clippy::too_many_arguments)] fn edit_stroke_width_ui( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_stroke_width = component - // TODO(#5607): what should happen if the promise is still pending? - .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; - let mut edit_stroke_width = current_stroke_width; - - let speed = (current_stroke_width * 0.01).at_least(0.001); - + value: &mut StrokeWidth, +) -> egui::Response { + let speed = (value.0 * 0.01).at_least(0.001); ui.add( - egui::DragValue::new(&mut edit_stroke_width) + egui::DragValue::new(&mut value.0) .clamp_range(0.0..=f64::INFINITY) .speed(speed), - ); - - if edit_stroke_width != current_stroke_width { - let new_stroke_width = StrokeWidth::from(edit_stroke_width); - - ctx.save_blueprint_component(override_path, &new_stroke_width); - } -} - -#[inline] -fn default_stroke_width( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> StrokeWidth { - StrokeWidth::from(1.0) + ) } -// ---- - -#[allow(clippy::too_many_arguments)] fn edit_marker_size_ui( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let current_marker_size = component - // TODO(#5607): what should happen if the promise is still pending? - .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; - let mut edit_marker_size = current_marker_size; - - let speed = (current_marker_size * 0.01).at_least(0.001); - + value: &mut MarkerSize, +) -> egui::Response { + let speed = (value.0 * 0.01).at_least(0.001); ui.add( - egui::DragValue::new(&mut edit_marker_size) + egui::DragValue::new(&mut value.0) .clamp_range(0.0..=f64::INFINITY) .speed(speed), - ); - - if edit_marker_size != current_marker_size { - let new_marker_size = MarkerSize::from(edit_marker_size); - - ctx.save_blueprint_component(override_path, &new_marker_size); - } -} - -#[inline] -fn default_marker_size( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> MarkerSize { - MarkerSize::from(1.0) + ) } // ---- -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, - ), -) where - C: Component + Loggable + 'static + Into<::std::borrow::Cow<'a, C>>, -{ - registry.add_editor( - C::name(), - Box::new(move |ctx, query, db, entity_path| { - let c = default(ctx, query, db, entity_path); - [c].into() - }), - Box::new(edit), - ); -} - /// Registers all editors of this crate in the component UI registry. /// /// ⚠️ This is supposed to be the only export of this crate. /// This crate is meant to be a leaf crate in the viewer ecosystem and should only be used by the `re_viewer` crate itself. pub fn register_editors(registry: &mut re_viewer_context::ComponentUiRegistry) { - register_editor::(registry, default_color, edit_color_ui); - register_editor::( - registry, - corner2d::default_corner2d, - corner2d::edit_corner2d, - ); - register_editor::(registry, default_marker_shape, edit_marker_shape_ui); - register_editor::(registry, default_marker_size, edit_marker_size_ui); - register_editor::(registry, default_name, edit_name_ui); - register_editor::(registry, default_radius, edit_radius_ui); - register_editor::(registry, default_scatter, edit_scatter_ui); - register_editor::(registry, default_stroke_width, edit_stroke_width_ui); - register_editor::(registry, default_text, edit_text_ui); - register_editor::(registry, visible::default_visible, visible::edit_visible); + registry.add_editor(edit_color_ui); + registry.add_editor(corner2d::edit_corner2d); + registry.add_editor(marker_shape::edit_marker_shape_ui); + registry.add_editor(edit_marker_size_ui); + registry.add_editor(edit_name_ui); + registry.add_editor(edit_radius_ui); + registry.add_editor(edit_scatter_ui); + registry.add_editor(edit_stroke_width_ui); + registry.add_editor(edit_text_ui); + registry.add_editor(visible::edit_visible); } diff --git a/crates/re_edit_ui/src/marker_shape.rs b/crates/re_edit_ui/src/marker_shape.rs new file mode 100644 index 000000000000..8b625dbc2bb1 --- /dev/null +++ b/crates/re_edit_ui/src/marker_shape.rs @@ -0,0 +1,85 @@ +use egui::NumExt as _; + +use re_types::components::MarkerShape; +use re_viewer_context::ViewerContext; + +pub(crate) fn edit_marker_shape_ui( + ctx: &ViewerContext<'_>, + ui: &mut egui::Ui, + edit_marker: &mut MarkerShape, +) -> egui::Response { + let marker_text = edit_marker.to_string(); // TODO(emilk): Show marker shape in the selected text + + let item_width = 100.0; + + let outer_response = egui::ComboBox::from_id_source("marker_shape") + .selected_text(marker_text) + .width( + ui.available_width() + .at_most(item_width + ui.spacing().menu_margin.sum().x), + ) + .height(320.0) + .show_ui(ui, |ui| { + // workaround to force `ui.max_rect()` to reflect the content size + ui.set_width(item_width); + + let background_x_range = (ui.max_rect() + ui.spacing().menu_margin).x_range(); + + let list_ui = |ui: &mut egui::Ui| { + let mut combined_response: Option = None; + for marker in MarkerShape::ALL { + let mut response = ctx + .re_ui + .list_item() + .selected(*edit_marker == marker) + .show_flat( + ui, + re_ui::list_item::LabelContent::new(marker.to_string()) + .min_desired_width(item_width) + .with_icon_fn(|_re_ui, ui, rect, visuals| { + paint_marker(ui, marker.into(), rect, visuals.text_color()); + }), + ); + + if response.clicked() { + *edit_marker = marker; + response.changed = true; + } + + combined_response = Some(match combined_response { + Some(combined_response) => combined_response.union(response), + None => response, + }); + } + combined_response.expect("At least one marker shape should be available") + }; + + re_ui::full_span::full_span_scope(ui, background_x_range, |ui| { + re_ui::list_item::list_item_scope(ui, "marker_shape", list_ui) + }) + }); + + outer_response.inner.unwrap_or(outer_response.response) +} + +pub(crate) fn paint_marker( + ui: &egui::Ui, + marker: egui_plot::MarkerShape, + rect: egui::Rect, + color: egui::Color32, +) { + use egui_plot::PlotItem as _; + + let points = egui_plot::Points::new([0.0, 0.0]) + .shape(marker) + .color(color) + .radius(rect.size().min_elem() / 2.0) + .filled(true); + + let bounds = egui_plot::PlotBounds::new_symmetrical(0.5); + let transform = egui_plot::PlotTransform::new(rect, bounds, true, true); + + let mut shapes = vec![]; + points.shapes(ui, &transform, &mut shapes); + ui.painter().extend(shapes); +} diff --git a/crates/re_edit_ui/src/visible.rs b/crates/re_edit_ui/src/visible.rs index a9527549a5e1..6e2ef2e3d45f 100644 --- a/crates/re_edit_ui/src/visible.rs +++ b/crates/re_edit_ui/src/visible.rs @@ -1,44 +1,15 @@ -use re_data_store::LatestAtQuery; -use re_entity_db::{external::re_query::LatestAtComponentResults, EntityDb}; -use re_log_types::{EntityPath, Instance}; use re_types::blueprint::components::Visible; -use re_viewer_context::{UiLayout, ViewerContext}; +use re_viewer_context::ViewerContext; -#[allow(clippy::too_many_arguments)] pub fn edit_visible( - ctx: &ViewerContext<'_>, + _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - instance: &Instance, -) { - let visible = component - // TODO(#5607): what should happen if the promise is still pending? - .try_instance::(db.resolver(), instance.get() as _) - .unwrap_or_else(|| default_visible(ctx, query, db, entity_path)); - let mut edit_visible = visible; - + value: &mut Visible, +) -> egui::Response { ui.scope(|ui| { ui.visuals_mut().widgets.hovered.expansion = 0.0; ui.visuals_mut().widgets.active.expansion = 0.0; - ui.add(re_ui::toggle_switch(15.0, &mut edit_visible.0)); - }); - - if edit_visible != visible { - ctx.save_blueprint_component(override_path, &edit_visible); - } -} - -#[inline] -pub fn default_visible( - _ctx: &ViewerContext<'_>, - _query: &LatestAtQuery, - _db: &EntityDb, - _entity_path: &EntityPath, -) -> Visible { - Visible(true) + ui.add(re_ui::toggle_switch(15.0, &mut value.0)) + }) + .inner } diff --git a/crates/re_selection_panel/src/override_ui.rs b/crates/re_selection_panel/src/override_ui.rs index ef4fa942fb84..56ac75075f9f 100644 --- a/crates/re_selection_panel/src/override_ui.rs +++ b/crates/re_selection_panel/src/override_ui.rs @@ -7,9 +7,8 @@ use re_entity_db::{EntityDb, InstancePath}; use re_log_types::{DataCell, DataRow, RowId, StoreKind}; use re_types_core::{components::VisualizerOverrides, ComponentName}; use re_viewer_context::{ - ComponentFallbackResult, DataResult, FallbackProviderContext, OverridePath, - SpaceViewClassExt as _, SystemCommand, SystemCommandSender as _, UiLayout, - ViewSystemIdentifier, ViewerContext, + ComponentFallbackResult, DataResult, OverridePath, QueryContext, SpaceViewClassExt as _, + SystemCommand, SystemCommandSender as _, UiLayout, ViewSystemIdentifier, ViewerContext, }; use re_viewport_blueprint::SpaceViewBlueprint; @@ -21,7 +20,7 @@ pub fn override_ui( ) { let InstancePath { entity_path, - instance, + instance: _, // Override ui only works on the first instance of an entity. } = instance_path; // Because of how overrides are implemented the overridden-data must be an entity @@ -53,7 +52,7 @@ pub fn override_ui( let mut component_to_vis: BTreeMap = Default::default(); // Accumulate the components across all visualizers and track which visualizer - // each component came from so we can use it for defaults later. + // each component came from so we can use it for fallbacks later. // // If two visualizers have the same component, the first one wins. // TODO(jleibs): We can do something fancier in the future such as presenting both @@ -87,11 +86,10 @@ pub fn override_ui( return; }; - let components = overrides + let sorted_overrides = overrides .resolved_component_overrides .into_iter() - .sorted_by_key(|(c, _)| *c) - .filter(|(c, _)| component_to_vis.contains_key(c)); + .sorted_by_key(|(c, _)| *c); re_ui::list_item::list_item_scope(ui, "overrides", |ui| { ui.spacing_mut().item_spacing.y = 0.0; @@ -101,48 +99,53 @@ pub fn override_ui( ref store_kind, path: ref entity_path_overridden, }, - ) in components + ) in sorted_overrides { + let Some(visualizer_identifier) = component_to_vis.get(component_name) else { + continue; + }; + let Ok(visualizer) = view_systems.get_by_identifier(*visualizer_identifier) else { + re_log::warn!( + "Failed to resolve visualizer identifier {visualizer_identifier}, to a visualizer implementation" + ); + continue; + }; + let value_fn = |ui: &mut egui::Ui| { - let component_data = match store_kind { + let (origin_db, query) = match store_kind { StoreKind::Blueprint => { - let store = ctx.store_context.blueprint.store(); - let query = ctx.blueprint_query; - ctx.store_context - .blueprint - .query_caches() - .latest_at(store, query, entity_path_overridden, [*component_name]) - .components - .get(component_name) - .cloned() /* arc */ - } - StoreKind::Recording => { - ctx.recording() - .query_caches() - .latest_at( - ctx.recording_store(), - &query, - entity_path_overridden, - [*component_name], - ) - .components - .get(component_name) - .cloned() /* arc */ + (ctx.store_context.blueprint, ctx.blueprint_query.clone()) } + StoreKind::Recording => (ctx.recording(), ctx.current_query()), }; + let component_data = origin_db + .query_caches() + .latest_at( + origin_db.store(), + &query, + entity_path_overridden, + [*component_name], + ) + .components + .get(component_name) + .cloned(); /* arc */ if let Some(results) = component_data { ctx.component_ui_registry.edit_ui( - ctx, + &QueryContext { + viewer_ctx: ctx, + target_entity_path: entity_path_overridden, + archetype_name: None, + query: &query, + }, ui, UiLayout::List, - &query, - ctx.recording(), + origin_db, + &instance_path.entity_path, entity_path_overridden, - &overrides.individual_override_path, + *component_name, &results, - component_name, - instance, + visualizer.as_fallback_provider(), ); } else { // TODO(jleibs): Is it possible to set an override to empty and not confuse @@ -247,15 +250,6 @@ pub fn add_new_override( } }) }) - .or_else(|| { - ctx.component_ui_registry.default_value( - ctx, - query, - db, - &data_result.entity_path, - component, - ) - }) else { re_log::warn!("Could not identify an initial value for: {}", component); return; diff --git a/crates/re_space_view/src/view_property_ui.rs b/crates/re_space_view/src/view_property_ui.rs index 61022dc751dd..abed4b08f5fc 100644 --- a/crates/re_space_view/src/view_property_ui.rs +++ b/crates/re_space_view/src/view_property_ui.rs @@ -1,7 +1,7 @@ use ahash::HashMap; use re_types_core::Archetype; use re_ui::list_item; -use re_viewer_context::{SpaceViewId, ViewerContext}; +use re_viewer_context::{ComponentFallbackProvider, QueryContext, SpaceViewId, ViewerContext}; use re_viewport_blueprint::entity_path_for_view_property; /// Display the UI for editing all components of a blueprint archetype. @@ -9,13 +9,21 @@ use re_viewport_blueprint::entity_path_for_view_property; /// Note that this will show default values for components that are null. pub fn view_property_ui( ctx: &ViewerContext<'_>, - space_view_id: SpaceViewId, ui: &mut egui::Ui, + space_view_id: SpaceViewId, + fallback_provider: &dyn ComponentFallbackProvider, ) { let blueprint_db = ctx.store_context.blueprint; let blueprint_query = ctx.blueprint_query; let blueprint_path = entity_path_for_view_property::(space_view_id, blueprint_db.tree()); + let query_ctx = QueryContext { + viewer_ctx: ctx, + target_entity_path: &blueprint_path, + archetype_name: Some(A::name()), + query: blueprint_query, + }; + let component_names = A::all_components(); let component_results = blueprint_db.latest_at( blueprint_query, @@ -53,16 +61,15 @@ pub fn view_property_ui( }) .value_fn(|_, ui, _| { ctx.component_ui_registry.edit_ui( - ctx, + &query_ctx, ui, re_viewer_context::UiLayout::List, - blueprint_query, blueprint_db, &blueprint_path, &blueprint_path, + *component_name, component_results.get_or_empty(*component_name), - component_name, - &0.into(), + fallback_provider, ); }), ); diff --git a/crates/re_space_view_time_series/src/space_view_class.rs b/crates/re_space_view_time_series/src/space_view_class.rs index 34478edd7ecb..78cff37cc723 100644 --- a/crates/re_space_view_time_series/src/space_view_class.rs +++ b/crates/re_space_view_time_series/src/space_view_class.rs @@ -7,6 +7,7 @@ use re_format::next_grid_tick_magnitude_ns; use re_log_types::{EntityPath, TimeInt, TimeZone}; use re_space_view::{controls, view_property_ui}; use re_types::blueprint::archetypes::PlotLegend; +use re_types::blueprint::components::Corner2D; use re_types::{components::Range1D, datatypes::TimeRange, SpaceViewClassIdentifier, View}; use re_ui::list_item; use re_viewer_context::external::re_entity_db::{ @@ -16,8 +17,8 @@ use re_viewer_context::{ IdentifiedViewSystem, IndicatedEntities, PerVisualizer, QueryRange, RecommendedSpaceView, SmallVisualizerSet, SpaceViewClass, SpaceViewClassRegistryError, SpaceViewId, SpaceViewSpawnHeuristics, SpaceViewState, SpaceViewStateExt as _, - SpaceViewSystemExecutionError, SystemExecutionOutput, ViewQuery, ViewSystemIdentifier, - ViewerContext, VisualizableEntities, + SpaceViewSystemExecutionError, SystemExecutionOutput, TypedComponentFallbackProvider, + ViewQuery, ViewSystemIdentifier, ViewerContext, VisualizableEntities, }; use re_viewport_blueprint::query_view_property_or_default; @@ -186,7 +187,7 @@ impl SpaceViewClass for TimeSeriesSpaceView { (and readability) in such situations as it prevents overdraw.", ); - view_property_ui::(ctx, space_view_id, ui); + view_property_ui::(ctx, ui, space_view_id, self); axis_ui(ctx, space_view_id, ui, state); }); @@ -802,3 +803,13 @@ fn round_ns_to_start_of_day(ns: i64) -> i64 { let ns_per_day = 24 * 60 * 60 * 1_000_000_000; (ns + ns_per_day / 2) / ns_per_day * ns_per_day } + +impl TypedComponentFallbackProvider for TimeSeriesSpaceView { + fn fallback_value(&self, _ctx: &re_viewer_context::QueryContext<'_>) -> Corner2D { + // TODO: a static_fallback should do here? + Corner2D::RightBottom + } +} + +// TODO: implement some fallbacks +re_viewer_context::impl_component_fallback_provider!(TimeSeriesSpaceView => [Corner2D]); diff --git a/crates/re_viewer_context/Cargo.toml b/crates/re_viewer_context/Cargo.toml index f90f05f6eb63..18503e45de91 100644 --- a/crates/re_viewer_context/Cargo.toml +++ b/crates/re_viewer_context/Cargo.toml @@ -22,6 +22,7 @@ all-features = true re_data_source.workspace = true re_data_store.workspace = true re_entity_db = { workspace = true, features = ["serde"] } +re_error.workspace = true re_log_types.workspace = true re_log.workspace = true re_query.workspace = true diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 000c6c400e28..122b18a75385 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -2,10 +2,11 @@ use std::collections::BTreeMap; use re_data_store::LatestAtQuery; use re_entity_db::{external::re_query::LatestAtComponentResults, EntityDb, EntityPath}; -use re_log_types::{DataCell, Instance}; -use re_types::ComponentName; +use re_log::ResultExt; +use re_log_types::Instance; +use re_types::{external::arrow2, ComponentName}; -use crate::ViewerContext; +use crate::{ComponentFallbackProvider, ComponentFallbackResult, QueryContext, ViewerContext}; /// Specifies the context in which the UI is used and the constraints it should follow. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -52,31 +53,27 @@ type ComponentUiCallback = Box< + Sync, >; -type ComponentEditCallback = Box< +/// Callback for editing a component via ui. +/// +/// Draws a ui showing the current value and allows the user to edit it. +/// If any edit was made, should return `Some` with the updated value. +/// If no edit was made, should return `None`. +type UntypedComponentEditCallback = Box< dyn Fn( &ViewerContext<'_>, &mut egui::Ui, - UiLayout, - &LatestAtQuery, - &EntityDb, - &EntityPath, - &EntityPath, - &LatestAtComponentResults, - &Instance, - ) + Send + &dyn arrow2::array::Array, + ) -> Option> + + Send + Sync, >; -type DefaultValueCallback = Box< - dyn Fn(&ViewerContext<'_>, &LatestAtQuery, &EntityDb, &EntityPath) -> DataCell + Send + Sync, ->; - /// How to display components in a Ui. pub struct ComponentUiRegistry { /// Ui method to use if there was no specific one registered for a component. fallback_ui: ComponentUiCallback, component_uis: BTreeMap, - component_editors: BTreeMap, + component_editors: BTreeMap, } impl ComponentUiRegistry { @@ -101,14 +98,81 @@ impl ComponentUiRegistry { /// UI and save the updated value. /// /// If the component was already registered, the new callback replaces the old one. - pub fn add_editor( + pub fn add_untyped_editor( &mut self, name: ComponentName, - default_value: DefaultValueCallback, - editor_callback: ComponentEditCallback, + editor_callback: UntypedComponentEditCallback, + ) { + self.component_editors.insert(name, editor_callback); + } + + /// Registers how to edit a given component in the ui. + /// + /// If the component was already registered, the new callback replaces the old one. + /// + /// Typed editors do not handle absence of a value as well as lists of values and will be skipped in these cases. + /// (This means that there must always be at least a fallback value available.) + /// + /// The value is only updated if the editor callback returns a `egui::Response::changed`. + /// On the flip side, this means that even if the data has not changed it may be written back to the store. + /// This can be relevant for transitioning from a fallback or default value to a custom value even if they are equal. + /// + /// Design principles for writing editors: + /// * Don't show a tooltip, this is solved at a higher level. + /// * Try not to assume context of the component beyond its inherent semantics + /// (e.g. if you get a `Color` you can't assume whether it's a background color or a point color) + /// + /// TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2d bounds is too complex for a single line). + pub fn add_editor( + &mut self, + editor_callback: impl Fn(&ViewerContext<'_>, &mut egui::Ui, &mut C) -> egui::Response + + Send + + Sync + + 'static, ) { - self.component_editors - .insert(name, (default_value, editor_callback)); + let untyped_callback: UntypedComponentEditCallback = + Box::new(move |ui, ui_layout, value| { + let deserialized = C::from_arrow(value); + let mut deserialized_value = match deserialized { + Ok(values) => { + if values.len() > 1 { + // Whatever we did prior to calling this should have taken care if it! + re_log::error_once!( + "Can only edit a single value at a time, got {} values for editing {}", + values.len(), + C::name() + ); + } + if let Some(v) = values.into_iter().next() { + v + } else { + re_log::warn_once!( + "Editor ui for {} needs a start value to operate on.", + C::name() + ); + return None; + } + } + Err(err) => { + re_log::error_once!( + "Failed to deserialize component of type {}: {:?}", + C::name(), + err + ); + return None; + } + }; + + editor_callback(ui, ui_layout, &mut deserialized_value) + .changed() + .then(|| { + use re_types::LoggableBatch; + deserialized_value.to_arrow().ok_or_log_error_once() + }) + .flatten() + }); + + self.add_untyped_editor(C::name(), untyped_callback); } /// Check if there is a registered editor for a given component @@ -153,63 +217,105 @@ impl ComponentUiRegistry { } /// Show an editor for this instance of this component. + /// + /// Changes will be written to the blueprint store at the given override path. + /// Any change is expected to be effective next frame and passed in via the `component_query_result` parameter. + /// (Otherwise, this method is agnostic to where the component data is stored.) #[allow(clippy::too_many_arguments)] pub fn edit_ui( &self, - ctx: &ViewerContext<'_>, + ctx: &QueryContext<'_>, ui: &mut egui::Ui, ui_layout: UiLayout, - query: &LatestAtQuery, - db: &EntityDb, + origin_db: &EntityDb, entity_path: &EntityPath, - override_path: &EntityPath, - component: &LatestAtComponentResults, - component_name: &ComponentName, - instance: &Instance, + blueprint_override_path: &EntityPath, + component_name: ComponentName, + component_query_result: &LatestAtComponentResults, + fallback_provider: &dyn ComponentFallbackProvider, ) { re_tracing::profile_function!(component_name.full_name()); - if let Some((_, edit_callback)) = self.component_editors.get(component_name) { - (*edit_callback)( - ctx, - ui, - ui_layout, - query, - db, - entity_path, - override_path, - component, - instance, - ); + // TODO(andreas, jleibs): Editors only show & edit the first instance of a component batch. + let instance: Instance = 0.into(); + + if let Some(edit_callback) = self.component_editors.get(&component_name) { + let component_query_result = match component_query_result.resolved(origin_db.resolver()) + { + re_query::PromiseResult::Pending => { + ui.label("Loading component data..."); + return; + } + re_query::PromiseResult::Ready(cell) => { + let index = instance.get(); + if cell.num_instances() > index as u32 { + Some(cell.as_arrow_ref().sliced(index as usize, 1)) + } else { + None + } + } + re_query::PromiseResult::Error(err) => { + let error_text = re_error::format_ref(err.as_ref()); + re_log::error_once!("Couldn't get {component_name}: {error_text}"); + ui.label(ctx.viewer_ctx.re_ui.error_text("Error")) + .on_hover_text(error_text); + return; + } + }; + + let component_value_or_fallback = component_query_result.unwrap_or_else(|| { + match fallback_provider.fallback_value(ctx, component_name) { + ComponentFallbackResult::Value(value) => value, + ComponentFallbackResult::SerializationError(err) => { + re_log::error_once!( + "Failed to deserialize component of type {}: {:?}", + component_name, + err + ); + empty_arrow_component_array(origin_db, component_name) + } + ComponentFallbackResult::UnknownComponent => { + empty_arrow_component_array(origin_db, component_name) + } + } + }); + + if let Some(updated) = + (*edit_callback)(ctx.viewer_ctx, ui, component_value_or_fallback.as_ref()) + { + ctx.viewer_ctx.save_blueprint_data_cell( + blueprint_override_path, + re_log_types::DataCell::from_arrow(component_name, updated), + ); + } } else { // Even if we can't edit the component, it's still helpful to show what the value is. self.ui( - ctx, + ctx.viewer_ctx, ui, ui_layout, - query, - db, + ctx.query, + origin_db, entity_path, - component, - instance, + component_query_result, + &instance, ); } } +} - /// Return a default value for this component. - #[allow(clippy::too_many_arguments)] - pub fn default_value( - &self, - ctx: &ViewerContext<'_>, - query: &LatestAtQuery, - db: &EntityDb, - entity_path: &EntityPath, - component: &ComponentName, - ) -> Option { - re_tracing::profile_function!(component); +fn empty_arrow_component_array( + origin_db: &EntityDb, + component_name: ComponentName, +) -> Box { + let datatype = origin_db + .data_store() + .lookup_datatype(&component_name) + .cloned() + .unwrap_or_else(|| { + re_log::error!("Unknown component type {component_name}"); + arrow2::datatypes::DataType::Null + }); - self.component_editors - .get(component) - .map(|(default_value, _)| (*default_value)(ctx, query, db, entity_path)) - } + arrow2::array::new_empty_array(datatype) } From 8c037ca87b53eee994ce27ab51f40c9a5d362eb8 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 29 May 2024 13:19:11 +0200 Subject: [PATCH 06/24] rename component_fallback module --- ...{component_fallback_provider.rs => component_fallbacks.rs} | 0 crates/re_viewer_context/src/lib.rs | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename crates/re_viewer_context/src/{component_fallback_provider.rs => component_fallbacks.rs} (100%) diff --git a/crates/re_viewer_context/src/component_fallback_provider.rs b/crates/re_viewer_context/src/component_fallbacks.rs similarity index 100% rename from crates/re_viewer_context/src/component_fallback_provider.rs rename to crates/re_viewer_context/src/component_fallbacks.rs diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 017a399e94d4..79bde3c62b47 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -9,7 +9,7 @@ mod blueprint_id; mod caches; mod collapsed_id; mod command_sender; -mod component_fallback_provider; +mod component_fallbacks; mod component_ui_registry; mod contents; mod item; @@ -42,7 +42,7 @@ pub use collapsed_id::{CollapseItem, CollapseScope, CollapsedId}; pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; -pub use component_fallback_provider::{ +pub use component_fallbacks::{ ComponentFallbackProvider, ComponentFallbackResult, QueryContext, TypedComponentFallbackProvider, }; From 85b43d49fd5d7d0c017a7ba2ad1cc5c16c18e094 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 29 May 2024 13:55:20 +0200 Subject: [PATCH 07/24] move QueryContext to query_context module, some doc improvements --- .../src/component_fallbacks.rs | 40 +++++-------------- .../src/component_ui_registry.rs | 2 +- crates/re_viewer_context/src/lib.rs | 7 ++-- crates/re_viewer_context/src/query_context.rs | 24 +++++++++++ 4 files changed, 39 insertions(+), 34 deletions(-) diff --git a/crates/re_viewer_context/src/component_fallbacks.rs b/crates/re_viewer_context/src/component_fallbacks.rs index 3b7853722dbb..9001d1b9578d 100644 --- a/crates/re_viewer_context/src/component_fallbacks.rs +++ b/crates/re_viewer_context/src/component_fallbacks.rs @@ -1,12 +1,17 @@ use re_types::external::arrow2; -use crate::ViewerContext; +use crate::QueryContext; -// TODO: Docs +/// Result for a fallback request. pub enum ComponentFallbackResult { + /// A fallback value was successfully provided. Value(Box), + + /// Arrow serialization failed. SerializationError(re_types::SerializationError), - UnknownComponent, + + /// The fallback provider is not able to handle the given component. + ComponentNotHandled, } impl From for ComponentFallbackResult { @@ -18,31 +23,6 @@ impl From for ComponentFallbackResult { } } -/// Context for a latest at query in a specific view. -/// -/// TODO: move elsewhere -/// TODO: this is centered around latest-at queries. does it have to be -pub struct QueryContext<'a> { - pub viewer_ctx: &'a ViewerContext<'a>, - - /// Target entity path which is lacking the component and needs a fallback. - /// - /// For editing overrides/defaults, this is the path to the store entity where they override/default is used. - /// For view properties this is the path that stores the respective view property archetype. - pub target_entity_path: &'a re_log_types::EntityPath, - - /// Archetype name in which context the component is needed. - /// - /// View properties always have an archetype context, but overrides/defaults may not. - pub archetype_name: Option, - - /// Query which didn't yield a result for the component at the target entity path. - // TODO(andreas): Can we make this a `ViewQuery` instead? - // pub query: &'a ViewQuery<'a>, - pub query: &'a re_data_store::LatestAtQuery, - // pub view_state: &'a dyn SpaceViewState, // TODO(andreas): Need this, but don't know yet how to patch through everywhere. -} - // TODO: Docs pub trait ComponentFallbackProvider { // TODO: Docs @@ -51,7 +31,7 @@ pub trait ComponentFallbackProvider { _ctx: &QueryContext<'_>, _component: re_types::ComponentName, ) -> ComponentFallbackResult { - ComponentFallbackResult::UnknownComponent + ComponentFallbackResult::ComponentNotHandled } } @@ -75,7 +55,7 @@ macro_rules! impl_component_fallback_provider { return $crate::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); } )* - $crate::ComponentFallbackResult::UnknownComponent + $crate::ComponentFallbackResult::ComponentNotHandled } } }; diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 122b18a75385..3d01d87103cb 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -274,7 +274,7 @@ impl ComponentUiRegistry { ); empty_arrow_component_array(origin_db, component_name) } - ComponentFallbackResult::UnknownComponent => { + ComponentFallbackResult::ComponentNotHandled => { empty_arrow_component_array(origin_db, component_name) } } diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 79bde3c62b47..03461afc725b 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -43,13 +43,14 @@ pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; pub use component_fallbacks::{ - ComponentFallbackProvider, ComponentFallbackResult, QueryContext, - TypedComponentFallbackProvider, + ComponentFallbackProvider, ComponentFallbackResult, TypedComponentFallbackProvider, }; pub use component_ui_registry::{ComponentUiRegistry, UiLayout}; pub use contents::{blueprint_id_to_tile_id, Contents, ContentsName}; pub use item::Item; -pub use query_context::{DataQueryResult, DataResultHandle, DataResultNode, DataResultTree}; +pub use query_context::{ + DataQueryResult, DataResultHandle, DataResultNode, DataResultTree, QueryContext, +}; pub use query_range::QueryRange; pub use selection_history::SelectionHistory; pub use selection_state::{ diff --git a/crates/re_viewer_context/src/query_context.rs b/crates/re_viewer_context/src/query_context.rs index 2b5302a8f560..0cde9a4f359b 100644 --- a/crates/re_viewer_context/src/query_context.rs +++ b/crates/re_viewer_context/src/query_context.rs @@ -12,6 +12,30 @@ slotmap::new_key_type! { pub struct DataResultHandle; } +/// Context for a latest at query in a specific view. +/// +// TODO(andreas) this is centered around latest-at queries. Does it have to be? Makes sense for UI, but that means it won't scale much into Visualizer queriers. +pub struct QueryContext<'a> { + pub viewer_ctx: &'a ViewerContext<'a>, + + /// Target entity path which is lacking the component and needs a fallback. + /// + /// For editing overrides/defaults, this is the path to the store entity where they override/default is used. + /// For view properties this is the path that stores the respective view property archetype. + pub target_entity_path: &'a re_log_types::EntityPath, + + /// Archetype name in which context the component is needed. + /// + /// View properties always have an archetype context, but overrides/defaults may not. + pub archetype_name: Option, + + /// Query which didn't yield a result for the component at the target entity path. + // TODO(andreas): Can we make this a `ViewQuery` instead? + // pub query: &'a ViewQuery<'a>, + pub query: &'a re_data_store::LatestAtQuery, + // pub view_state: &'a dyn SpaceViewState, // TODO(andreas): Need this, but don't know yet how to patch through everywhere. +} + /// The result of executing a single data query #[derive(Default)] pub struct DataQueryResult { From 75788019d4d561765a67ea34345f7d4a644009f2 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 13:29:10 +0200 Subject: [PATCH 08/24] integrate base fallback into component fallback system --- crates/re_selection_panel/src/override_ui.rs | 29 +++---- .../src/visualizer_system.rs | 10 ++- crates/re_viewer/src/app.rs | 21 +++++ crates/re_viewer/src/app_state.rs | 3 + .../src/component_fallbacks.rs | 78 +++++++++++++++---- .../src/component_ui_registry.rs | 44 ++++------- crates/re_viewer_context/src/lib.rs | 3 +- crates/re_viewer_context/src/test_context.rs | 1 + .../re_viewer_context/src/viewer_context.rs | 14 +++- 9 files changed, 134 insertions(+), 69 deletions(-) diff --git a/crates/re_selection_panel/src/override_ui.rs b/crates/re_selection_panel/src/override_ui.rs index 56ac75075f9f..16014cbe1ae3 100644 --- a/crates/re_selection_panel/src/override_ui.rs +++ b/crates/re_selection_panel/src/override_ui.rs @@ -7,8 +7,8 @@ use re_entity_db::{EntityDb, InstancePath}; use re_log_types::{DataCell, DataRow, RowId, StoreKind}; use re_types_core::{components::VisualizerOverrides, ComponentName}; use re_viewer_context::{ - ComponentFallbackResult, DataResult, OverridePath, QueryContext, SpaceViewClassExt as _, - SystemCommand, SystemCommandSender as _, UiLayout, ViewSystemIdentifier, ViewerContext, + DataResult, OverridePath, QueryContext, SpaceViewClassExt as _, SystemCommand, + SystemCommandSender as _, UiLayout, ViewSystemIdentifier, ViewerContext, }; use re_viewport_blueprint::SpaceViewBlueprint; @@ -199,6 +199,13 @@ pub fn add_new_override( opened = true; ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); + let query_context = QueryContext { + viewer_ctx: ctx, + target_entity_path: &data_result.entity_path, + archetype_name: None, + query, + }; + // Present the option to add new components for each component that doesn't // already have an active override. for (component, viz) in component_to_vis { @@ -233,21 +240,9 @@ pub fn add_new_override( .and_then(|result| result.2[0].clone()) .or_else(|| { view_systems.get_by_identifier(*viz).ok().and_then(|sys| { - if let ComponentFallbackResult::Value(value) = sys - .fallback_value( - &QueryContext { - viewer_ctx: ctx, - target_entity_path: &data_result.entity_path, - archetype_name: None, - query, - }, - *component, - ) - { - Some(DataCell::from_arrow(*component, value)) - } else { - None - } + sys.fallback_for(&query_context, *component) + .map(|fallback| DataCell::from_arrow(*component, fallback)) + .ok() }) }) else { diff --git a/crates/re_space_view_dataframe/src/visualizer_system.rs b/crates/re_space_view_dataframe/src/visualizer_system.rs index b316d5c07743..350f161f00da 100644 --- a/crates/re_space_view_dataframe/src/visualizer_system.rs +++ b/crates/re_space_view_dataframe/src/visualizer_system.rs @@ -36,4 +36,12 @@ impl VisualizerSystem for EmptySystem { } } -impl ComponentFallbackProvider for EmptySystem {} +impl ComponentFallbackProvider for EmptySystem { + fn try_provide_fallback( + &self, + _ctx: &re_viewer_context::QueryContext<'_>, + _component: re_types_core::ComponentName, + ) -> re_viewer_context::ComponentFallbackProviderResult { + re_viewer_context::ComponentFallbackProviderResult::ComponentNotHandled + } +} diff --git a/crates/re_viewer/src/app.rs b/crates/re_viewer/src/app.rs index 1ff03c4dcf76..b176b60e5c0c 100644 --- a/crates/re_viewer/src/app.rs +++ b/crates/re_viewer/src/app.rs @@ -163,6 +163,11 @@ pub struct App { pub(crate) panel_state_overrides_active: bool, pub(crate) panel_state_overrides: PanelStateOverrides, + + /// Lookup table for component base fallbacks. + /// + /// Base fallbacks are the default values for components that are used when no other context specific fallback is available. + component_base_fallbacks: re_viewer_context::ComponentBaseFallbacks, } impl App { @@ -241,6 +246,19 @@ impl App { let panel_state_overrides = startup_options.panel_state_overrides; + let component_base_fallbacks = { + re_tracing::profile_scope!("component_base_fallbacks"); + match crate::component_defaults::list_default_components() { + Ok(defaults) => defaults.collect(), + Err(err) => { + re_log::error!( + "Failed to create list of serialized default values for components: {err}" + ); + Default::default() + } + } + }; + Self { build_info, startup_options, @@ -283,6 +301,8 @@ impl App { panel_state_overrides_active: true, panel_state_overrides, + + component_base_fallbacks, } } @@ -950,6 +970,7 @@ impl App { hide: self.startup_options.hide_welcome_screen, opacity: self.welcome_screen_opacity(egui_ctx), }, + &self.component_base_fallbacks, ); } render_ctx.before_submit(); diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 623aca0bc9af..e9a692f59348 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -131,6 +131,7 @@ impl AppState { rx: &ReceiveSet, command_sender: &CommandSender, welcome_screen_state: &WelcomeScreenState, + component_base_fallbacks: &re_viewer_context::ComponentBaseFallbacks, ) { re_tracing::profile_function!(); @@ -256,6 +257,7 @@ impl AppState { render_ctx: Some(render_ctx), command_sender, focused_item, + component_base_fallbacks, }; // First update the viewport and thus all active space views. @@ -319,6 +321,7 @@ impl AppState { render_ctx: Some(render_ctx), command_sender, focused_item, + component_base_fallbacks, }; // diff --git a/crates/re_viewer_context/src/component_fallbacks.rs b/crates/re_viewer_context/src/component_fallbacks.rs index 9001d1b9578d..c42ed58225bf 100644 --- a/crates/re_viewer_context/src/component_fallbacks.rs +++ b/crates/re_viewer_context/src/component_fallbacks.rs @@ -1,20 +1,30 @@ -use re_types::external::arrow2; +use ahash::HashMap; +use re_types::{external::arrow2, ComponentName}; use crate::QueryContext; -/// Result for a fallback request. -pub enum ComponentFallbackResult { +/// Lookup table for component base fallbacks. +/// +/// Base fallbacks are the default values for components that are used when no other context specific fallback is available. +pub type ComponentBaseFallbacks = HashMap>; + +/// Result for a fallback request to a provider. +pub enum ComponentFallbackProviderResult { /// A fallback value was successfully provided. Value(Box), - /// Arrow serialization failed. - SerializationError(re_types::SerializationError), - /// The fallback provider is not able to handle the given component. + /// + /// This is not treated as an error and should be handled by looking up a base fallback. ComponentNotHandled, + + /// Arrow serialization failed. + /// + /// Unlike [`ComponentFallbackProviderResult::ComponentNotHandled`], this is treated as an unexpected error. + SerializationError(re_types::SerializationError), } -impl From for ComponentFallbackResult { +impl From for ComponentFallbackProviderResult { fn from(batch: T) -> Self { match batch.to_arrow() { Ok(value) => Self::Value(value), @@ -23,15 +33,51 @@ impl From for ComponentFallbackResult { } } +/// Result for a fallback request. +pub enum ComponentFallbackError { + /// The fallback provider is not able to handle the given component _and_ there was no base fallback. + /// This should never happen, since all components should have a base fallback. + MissingBaseFallback, +} + // TODO: Docs pub trait ComponentFallbackProvider { - // TODO: Docs - fn fallback_value( + /// Tries to provide a fallback value for a given component. + /// + /// If the provider can't handle the component or simply want to use a base fallback, + /// it should return `ComponentFallbackProviderResult::ComponentNotHandled`. + /// + /// Fallbacks can be based on arbitrarily complex & context sensitive heuristics. + fn try_provide_fallback( &self, - _ctx: &QueryContext<'_>, - _component: re_types::ComponentName, - ) -> ComponentFallbackResult { - ComponentFallbackResult::ComponentNotHandled + ctx: &QueryContext<'_>, + component: ComponentName, + ) -> ComponentFallbackProviderResult; + + /// Provides a fallback value for a given component, first trying the provider and then falling back to the base fallbacks. + fn fallback_for( + &self, + ctx: &QueryContext<'_>, + component: ComponentName, + ) -> Result, ComponentFallbackError> { + match self.try_provide_fallback(ctx, component) { + ComponentFallbackProviderResult::Value(value) => { + return Ok(value); + } + ComponentFallbackProviderResult::SerializationError(err) => { + // We still want to provide the base fallback value so we can move on, + // but arrow serialization should never fail. + // Giving out _both_ the error and the fallback value gets messy, + // so given that this should be a rare bug, we log it and return the fallback value as success. + re_log::error_once!("Arrow serialization failed trying to provide a fallback for {:?}. Using base fallback instead: {}", component, err); + } + ComponentFallbackProviderResult::ComponentNotHandled => {} + } + + match ctx.viewer_ctx.component_base_fallbacks.get(&component) { + Some(fallback) => Ok(fallback.clone()), + None => Err(ComponentFallbackError::MissingBaseFallback), + } } } @@ -45,17 +91,17 @@ pub trait TypedComponentFallbackProvider { macro_rules! impl_component_fallback_provider { ($type:ty => [$($component:ty),*]) => { impl $crate::ComponentFallbackProvider for $type { - fn fallback_value( + fn try_provide_fallback( &self, ctx: &$crate::QueryContext<'_>, component_name: re_types::ComponentName, - ) -> $crate::ComponentFallbackResult { + ) -> $crate::ComponentFallbackProviderResult { $( if component_name == <$component as re_types::Loggable>::name() { return $crate::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); } )* - $crate::ComponentFallbackResult::ComponentNotHandled + $crate::ComponentFallbackProviderResult::ComponentNotHandled } } }; diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 3d01d87103cb..596d73cd0bb4 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -6,7 +6,7 @@ use re_log::ResultExt; use re_log_types::Instance; use re_types::{external::arrow2, ComponentName}; -use crate::{ComponentFallbackProvider, ComponentFallbackResult, QueryContext, ViewerContext}; +use crate::{ComponentFallbackError, ComponentFallbackProvider, QueryContext, ViewerContext}; /// Specifies the context in which the UI is used and the constraints it should follow. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -263,22 +263,20 @@ impl ComponentUiRegistry { } }; - let component_value_or_fallback = component_query_result.unwrap_or_else(|| { - match fallback_provider.fallback_value(ctx, component_name) { - ComponentFallbackResult::Value(value) => value, - ComponentFallbackResult::SerializationError(err) => { - re_log::error_once!( - "Failed to deserialize component of type {}: {:?}", - component_name, - err - ); - empty_arrow_component_array(origin_db, component_name) - } - ComponentFallbackResult::ComponentNotHandled => { - empty_arrow_component_array(origin_db, component_name) + let component_value_or_fallback = if let Some(result) = component_query_result { + result + } else { + match fallback_provider.fallback_for(ctx, component_name) { + Ok(fallback) => fallback, + Err(ComponentFallbackError::MissingBaseFallback) => { + let error = format!("No fallback value available for {component_name}."); + re_log::error_once!("{error}"); + ui.label(ctx.viewer_ctx.re_ui.error_text("")) + .on_hover_text(error); + return; } } - }); + }; if let Some(updated) = (*edit_callback)(ctx.viewer_ctx, ui, component_value_or_fallback.as_ref()) @@ -303,19 +301,3 @@ impl ComponentUiRegistry { } } } - -fn empty_arrow_component_array( - origin_db: &EntityDb, - component_name: ComponentName, -) -> Box { - let datatype = origin_db - .data_store() - .lookup_datatype(&component_name) - .cloned() - .unwrap_or_else(|| { - re_log::error!("Unknown component type {component_name}"); - arrow2::datatypes::DataType::Null - }); - - arrow2::array::new_empty_array(datatype) -} diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 03461afc725b..3cb803a9b6e6 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -43,7 +43,8 @@ pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; pub use component_fallbacks::{ - ComponentFallbackProvider, ComponentFallbackResult, TypedComponentFallbackProvider, + ComponentBaseFallbacks, ComponentFallbackError, ComponentFallbackProvider, + ComponentFallbackProviderResult, TypedComponentFallbackProvider, }; pub use component_ui_registry::{ComponentUiRegistry, UiLayout}; pub use contents::{blueprint_id_to_tile_id, Contents, ContentsName}; diff --git a/crates/re_viewer_context/src/test_context.rs b/crates/re_viewer_context/src/test_context.rs index 25b1a487fa5c..7a0a6dd91715 100644 --- a/crates/re_viewer_context/src/test_context.rs +++ b/crates/re_viewer_context/src/test_context.rs @@ -82,6 +82,7 @@ impl TestContext { render_ctx: None, command_sender: &command_sender, focused_item: &None, + component_base_fallbacks: &Default::default(), }; func(&ctx, ui); diff --git a/crates/re_viewer_context/src/viewer_context.rs b/crates/re_viewer_context/src/viewer_context.rs index 2a7bcc595ffb..55f6c78f969d 100644 --- a/crates/re_viewer_context/src/viewer_context.rs +++ b/crates/re_viewer_context/src/viewer_context.rs @@ -5,9 +5,10 @@ use re_data_store::LatestAtQuery; use re_entity_db::entity_db::EntityDb; use crate::{ - query_context::DataQueryResult, AppOptions, ApplicableEntities, ApplicationSelectionState, - Caches, CommandSender, ComponentUiRegistry, IndicatedEntities, ItemCollection, PerVisualizer, - SpaceViewClassRegistry, SpaceViewId, StoreContext, SystemCommandSender as _, TimeControl, + component_fallbacks::ComponentBaseFallbacks, query_context::DataQueryResult, AppOptions, + ApplicableEntities, ApplicationSelectionState, Caches, CommandSender, ComponentUiRegistry, + IndicatedEntities, ItemCollection, PerVisualizer, SpaceViewClassRegistry, SpaceViewId, + StoreContext, SystemCommandSender as _, TimeControl, }; /// Common things needed by many parts of the viewer. @@ -69,6 +70,13 @@ pub struct ViewerContext<'a> { /// The focused item is cleared every frame, but views may react with side-effects /// that last several frames. pub focused_item: &'a Option, + + /// Fallback values for components to be used when [`crate::ComponentFallbackProvider::try_provide_fallback`] + /// is not able to provide a value. + /// + /// ⚠️ In almost all cases you should not use this directly, but instead use the currently best fitting + /// [`crate::ComponentFallbackProvider`] and call [`crate::ComponentFallbackProvider::fallback_for`] instead. + pub component_base_fallbacks: &'a ComponentBaseFallbacks, } impl<'a> ViewerContext<'a> { From 1f821182d0e82bf1f4c628c7c5a31b55975e99d9 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 16:13:48 +0200 Subject: [PATCH 09/24] rename base fallback to placeholder, fill various documentation gaps --- .../src/line_visualizer_system.rs | 4 +- .../src/point_visualizer_system.rs | 4 +- .../src/space_view_class.rs | 2 +- crates/re_viewer/src/app.rs | 10 ++--- crates/re_viewer/src/app_state.rs | 6 +-- .../src/component_fallbacks.rs | 40 ++++++++++++------- crates/re_viewer_context/src/lib.rs | 4 +- .../src/space_view/space_view_class.rs | 16 -------- crates/re_viewer_context/src/test_context.rs | 2 +- .../re_viewer_context/src/viewer_context.rs | 6 +-- .../color_coordinates_visualizer_system.rs | 3 +- 11 files changed, 45 insertions(+), 52 deletions(-) diff --git a/crates/re_space_view_time_series/src/line_visualizer_system.rs b/crates/re_space_view_time_series/src/line_visualizer_system.rs index 5087f5062b67..e11a564bd287 100644 --- a/crates/re_space_view_time_series/src/line_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/line_visualizer_system.rs @@ -77,13 +77,13 @@ impl VisualizerSystem for SeriesLineSystem { } impl TypedComponentFallbackProvider for SeriesLineSystem { - fn fallback_value(&self, ctx: &QueryContext<'_>) -> Color { + fn fallback_for(&self, ctx: &QueryContext<'_>) -> Color { initial_override_color(ctx.target_entity_path) } } impl TypedComponentFallbackProvider for SeriesLineSystem { - fn fallback_value(&self, _ctx: &QueryContext<'_>) -> StrokeWidth { + fn fallback_for(&self, _ctx: &QueryContext<'_>) -> StrokeWidth { StrokeWidth(DEFAULT_STROKE_WIDTH) } } diff --git a/crates/re_space_view_time_series/src/point_visualizer_system.rs b/crates/re_space_view_time_series/src/point_visualizer_system.rs index 410d5ebdf08f..946c9fbde25d 100644 --- a/crates/re_space_view_time_series/src/point_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/point_visualizer_system.rs @@ -80,13 +80,13 @@ impl VisualizerSystem for SeriesPointSystem { } impl TypedComponentFallbackProvider for SeriesPointSystem { - fn fallback_value(&self, ctx: &QueryContext<'_>) -> Color { + fn fallback_for(&self, ctx: &QueryContext<'_>) -> Color { initial_override_color(ctx.target_entity_path) } } impl TypedComponentFallbackProvider for SeriesPointSystem { - fn fallback_value(&self, _ctx: &QueryContext<'_>) -> MarkerSize { + fn fallback_for(&self, _ctx: &QueryContext<'_>) -> MarkerSize { MarkerSize(DEFAULT_MARKER_SIZE) } } diff --git a/crates/re_space_view_time_series/src/space_view_class.rs b/crates/re_space_view_time_series/src/space_view_class.rs index 78cff37cc723..12d13b705c14 100644 --- a/crates/re_space_view_time_series/src/space_view_class.rs +++ b/crates/re_space_view_time_series/src/space_view_class.rs @@ -805,7 +805,7 @@ fn round_ns_to_start_of_day(ns: i64) -> i64 { } impl TypedComponentFallbackProvider for TimeSeriesSpaceView { - fn fallback_value(&self, _ctx: &re_viewer_context::QueryContext<'_>) -> Corner2D { + fn fallback_for(&self, _ctx: &re_viewer_context::QueryContext<'_>) -> Corner2D { // TODO: a static_fallback should do here? Corner2D::RightBottom } diff --git a/crates/re_viewer/src/app.rs b/crates/re_viewer/src/app.rs index b176b60e5c0c..ab82273e570a 100644 --- a/crates/re_viewer/src/app.rs +++ b/crates/re_viewer/src/app.rs @@ -164,10 +164,8 @@ pub struct App { pub(crate) panel_state_overrides_active: bool, pub(crate) panel_state_overrides: PanelStateOverrides, - /// Lookup table for component base fallbacks. - /// - /// Base fallbacks are the default values for components that are used when no other context specific fallback is available. - component_base_fallbacks: re_viewer_context::ComponentBaseFallbacks, + /// Lookup table for component placeholder values, used whenever no fallback was provided explicitly. + component_placeholders: re_viewer_context::ComponentPlaceholders, } impl App { @@ -302,7 +300,7 @@ impl App { panel_state_overrides_active: true, panel_state_overrides, - component_base_fallbacks, + component_placeholders: component_base_fallbacks, } } @@ -970,7 +968,7 @@ impl App { hide: self.startup_options.hide_welcome_screen, opacity: self.welcome_screen_opacity(egui_ctx), }, - &self.component_base_fallbacks, + &self.component_placeholders, ); } render_ctx.before_submit(); diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index e9a692f59348..acf5f7c6ca2d 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -131,7 +131,7 @@ impl AppState { rx: &ReceiveSet, command_sender: &CommandSender, welcome_screen_state: &WelcomeScreenState, - component_base_fallbacks: &re_viewer_context::ComponentBaseFallbacks, + component_placeholders: &re_viewer_context::ComponentPlaceholders, ) { re_tracing::profile_function!(); @@ -257,7 +257,7 @@ impl AppState { render_ctx: Some(render_ctx), command_sender, focused_item, - component_base_fallbacks, + component_placeholders, }; // First update the viewport and thus all active space views. @@ -321,7 +321,7 @@ impl AppState { render_ctx: Some(render_ctx), command_sender, focused_item, - component_base_fallbacks, + component_placeholders, }; // diff --git a/crates/re_viewer_context/src/component_fallbacks.rs b/crates/re_viewer_context/src/component_fallbacks.rs index c42ed58225bf..cb89490ff475 100644 --- a/crates/re_viewer_context/src/component_fallbacks.rs +++ b/crates/re_viewer_context/src/component_fallbacks.rs @@ -3,10 +3,8 @@ use re_types::{external::arrow2, ComponentName}; use crate::QueryContext; -/// Lookup table for component base fallbacks. -/// -/// Base fallbacks are the default values for components that are used when no other context specific fallback is available. -pub type ComponentBaseFallbacks = HashMap>; +/// Lookup table for component placeholder values, used whenever no fallback was provided explicitly. +pub type ComponentPlaceholders = HashMap>; /// Result for a fallback request to a provider. pub enum ComponentFallbackProviderResult { @@ -15,7 +13,7 @@ pub enum ComponentFallbackProviderResult { /// The fallback provider is not able to handle the given component. /// - /// This is not treated as an error and should be handled by looking up a base fallback. + /// This is not treated as an error and should be handled by looking up a placeholder value. ComponentNotHandled, /// Arrow serialization failed. @@ -35,17 +33,19 @@ impl From for ComponentFallbackProviderResult { /// Result for a fallback request. pub enum ComponentFallbackError { - /// The fallback provider is not able to handle the given component _and_ there was no base fallback. - /// This should never happen, since all components should have a base fallback. + /// The fallback provider is not able to handle the given component _and_ there was no placeholder value. + /// This should never happen, since all components should have a placeholder value. MissingBaseFallback, } -// TODO: Docs +/// Provides fallback values for components, implemented typically by [`crate::SpaceViewClass`] and [`crate::VisualizerSystem`]. +/// +/// Fallbacks can be based on arbitrarily complex & context sensitive heuristics. pub trait ComponentFallbackProvider { /// Tries to provide a fallback value for a given component. /// - /// If the provider can't handle the component or simply want to use a base fallback, - /// it should return `ComponentFallbackProviderResult::ComponentNotHandled`. + /// If the provider can't handle the component or simply want to use a placeholder value, + /// it should return [`ComponentFallbackProviderResult::ComponentNotHandled`]. /// /// Fallbacks can be based on arbitrarily complex & context sensitive heuristics. fn try_provide_fallback( @@ -54,7 +54,8 @@ pub trait ComponentFallbackProvider { component: ComponentName, ) -> ComponentFallbackProviderResult; - /// Provides a fallback value for a given component, first trying the provider and then falling back to the base fallbacks. + /// Provides a fallback value for a given component, first trying the provider and + /// then falling back to the placeholder value registered in the viewer context. fn fallback_for( &self, ctx: &QueryContext<'_>, @@ -74,19 +75,28 @@ pub trait ComponentFallbackProvider { ComponentFallbackProviderResult::ComponentNotHandled => {} } - match ctx.viewer_ctx.component_base_fallbacks.get(&component) { + match ctx.viewer_ctx.component_placeholders.get(&component) { Some(fallback) => Ok(fallback.clone()), None => Err(ComponentFallbackError::MissingBaseFallback), } } } -// TODO: Docs +/// Provides a fallback value for a given component with known type. +/// +/// Use the [`crate::impl_component_fallback_provider`] macro to build a [`ComponentFallbackProvider`] +/// out of several strongly typed [`TypedComponentFallbackProvider`]s. pub trait TypedComponentFallbackProvider { - fn fallback_value(&self, ctx: &QueryContext<'_>) -> C; + fn fallback_for(&self, ctx: &QueryContext<'_>) -> C; } /// Implements the [`ComponentFallbackProvider`] trait for a given type, using a number of [`TypedComponentFallbackProvider`]. +/// +/// Usage examples: +/// ```no_run +/// impl_component_fallback_provider!(MySystem => []); // Empty fallback provider +/// impl_component_fallback_provider!(MySystem => [Color, Text]); // Fallback provider handling the Color and Text components. +/// ``` #[macro_export] macro_rules! impl_component_fallback_provider { ($type:ty => [$($component:ty),*]) => { @@ -98,7 +108,7 @@ macro_rules! impl_component_fallback_provider { ) -> $crate::ComponentFallbackProviderResult { $( if component_name == <$component as re_types::Loggable>::name() { - return $crate::TypedComponentFallbackProvider::<$component>::fallback_value(self, ctx).into(); + return $crate::TypedComponentFallbackProvider::<$component>::fallback_for(self, ctx).into(); } )* $crate::ComponentFallbackProviderResult::ComponentNotHandled diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 3cb803a9b6e6..8cd9d13492d0 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -43,8 +43,8 @@ pub use command_sender::{ command_channel, CommandReceiver, CommandSender, SystemCommand, SystemCommandSender, }; pub use component_fallbacks::{ - ComponentBaseFallbacks, ComponentFallbackError, ComponentFallbackProvider, - ComponentFallbackProviderResult, TypedComponentFallbackProvider, + ComponentFallbackError, ComponentFallbackProvider, ComponentFallbackProviderResult, + ComponentPlaceholders, TypedComponentFallbackProvider, }; pub use component_ui_registry::{ComponentUiRegistry, UiLayout}; pub use contents::{blueprint_id_to_tile_id, Contents, ContentsName}; diff --git a/crates/re_viewer_context/src/space_view/space_view_class.rs b/crates/re_viewer_context/src/space_view/space_view_class.rs index 570d804438a4..3e8256680654 100644 --- a/crates/re_viewer_context/src/space_view/space_view_class.rs +++ b/crates/re_viewer_context/src/space_view/space_view_class.rs @@ -211,22 +211,6 @@ pub trait SpaceViewClass: Send + Sync { ) -> Result<(), SpaceViewSystemExecutionError>; } -// TODO: -// /// View classes are expected to be able to provide default values for any component of their property archetypes. -// impl ComponentFallbackProvider for dyn SpaceViewClass { -// fn fallback_value( -// &self, -// _ctx: &ViewerContext<'_>, -// _component: re_types::ComponentName, -// _archetype_name: Option, -// _entity: &re_log_types::EntityPath, -// _query: &ViewQuery<'_>, -// _view_state: &dyn SpaceViewState, -// ) -> Option> { -// None -// } -// } - pub trait SpaceViewClassExt<'a>: SpaceViewClass + 'a { /// Determines the set of visible entities for a given space view. // TODO(andreas): This should be part of the SpaceView's (non-blueprint) state. diff --git a/crates/re_viewer_context/src/test_context.rs b/crates/re_viewer_context/src/test_context.rs index 7a0a6dd91715..9dec8bc12b6e 100644 --- a/crates/re_viewer_context/src/test_context.rs +++ b/crates/re_viewer_context/src/test_context.rs @@ -82,7 +82,7 @@ impl TestContext { render_ctx: None, command_sender: &command_sender, focused_item: &None, - component_base_fallbacks: &Default::default(), + component_placeholders: &Default::default(), }; func(&ctx, ui); diff --git a/crates/re_viewer_context/src/viewer_context.rs b/crates/re_viewer_context/src/viewer_context.rs index 55f6c78f969d..1e53ff57e154 100644 --- a/crates/re_viewer_context/src/viewer_context.rs +++ b/crates/re_viewer_context/src/viewer_context.rs @@ -5,7 +5,7 @@ use re_data_store::LatestAtQuery; use re_entity_db::entity_db::EntityDb; use crate::{ - component_fallbacks::ComponentBaseFallbacks, query_context::DataQueryResult, AppOptions, + component_fallbacks::ComponentPlaceholders, query_context::DataQueryResult, AppOptions, ApplicableEntities, ApplicationSelectionState, Caches, CommandSender, ComponentUiRegistry, IndicatedEntities, ItemCollection, PerVisualizer, SpaceViewClassRegistry, SpaceViewId, StoreContext, SystemCommandSender as _, TimeControl, @@ -71,12 +71,12 @@ pub struct ViewerContext<'a> { /// that last several frames. pub focused_item: &'a Option, - /// Fallback values for components to be used when [`crate::ComponentFallbackProvider::try_provide_fallback`] + /// Placeholder values for components to be used when [`crate::ComponentFallbackProvider::try_provide_fallback`] /// is not able to provide a value. /// /// ⚠️ In almost all cases you should not use this directly, but instead use the currently best fitting /// [`crate::ComponentFallbackProvider`] and call [`crate::ComponentFallbackProvider::fallback_for`] instead. - pub component_base_fallbacks: &'a ComponentBaseFallbacks, + pub component_placeholders: &'a ComponentPlaceholders, } impl<'a> ViewerContext<'a> { diff --git a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs index 89db4510ee9f..b29cdf2508b4 100644 --- a/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs +++ b/examples/rust/custom_space_view/src/color_coordinates_visualizer_system.rs @@ -115,5 +115,6 @@ impl VisualizerSystem for InstanceColorSystem { } } -// TODO: document the role of this +// Implements a `ComponentFallbackProvider` trait for the `InstanceColorSystem`. +// It is left empty here but could be used to provides fallback values for optional components in case they're missing. re_viewer_context::impl_component_fallback_provider!(InstanceColorSystem => []); From c1bf0db54b45b854f3bdd59417c4c69c74a175c3 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 16:57:35 +0200 Subject: [PATCH 10/24] fixed / better error handling for edit_ui --- .../src/space_view_class.rs | 4 +-- crates/re_ui/src/lib.rs | 17 +++++++++++ .../src/component_ui_registry.rs | 30 ++++++++++++------- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/crates/re_space_view_time_series/src/space_view_class.rs b/crates/re_space_view_time_series/src/space_view_class.rs index 12d13b705c14..3670d2d045fb 100644 --- a/crates/re_space_view_time_series/src/space_view_class.rs +++ b/crates/re_space_view_time_series/src/space_view_class.rs @@ -806,10 +806,10 @@ fn round_ns_to_start_of_day(ns: i64) -> i64 { impl TypedComponentFallbackProvider for TimeSeriesSpaceView { fn fallback_for(&self, _ctx: &re_viewer_context::QueryContext<'_>) -> Corner2D { - // TODO: a static_fallback should do here? + // Explicitely pick RightCorner2D::RightBottom, we don't want to make this dependent on the (arbitrary) + // default of Corner2D Corner2D::RightBottom } } -// TODO: implement some fallbacks re_viewer_context::impl_component_fallback_provider!(TimeSeriesSpaceView => [Corner2D]); diff --git a/crates/re_ui/src/lib.rs b/crates/re_ui/src/lib.rs index 7bf5b18d33b5..41ec2d589678 100644 --- a/crates/re_ui/src/lib.rs +++ b/crates/re_ui/src/lib.rs @@ -275,6 +275,23 @@ impl ReUi { .color(style.visuals.error_fg_color) } + /// Shows a small error label with the given text on hover. + pub fn error_label(&self, ui: &mut egui::Ui, error_text: impl Into) -> egui::Response { + ui.label(self.error_text("Error")) + .on_hover_text(error_text.into()) + } + + /// Shows a small error label with the given text on hover and logs the error once to the console. + pub fn error_label_and_log_once( + &self, + ui: &mut egui::Ui, + error_text: impl Into, + ) -> egui::Response { + let error_text = error_text.into(); + re_log::error_once!("{error_text}"); + self.error_label(ui, error_text) + } + /// The color we use to mean "loop this selection" pub fn loop_selection_color() -> egui::Color32 { egui::Color32::from_rgb(1, 37, 105) // from figma 2023-02-09 diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 596d73cd0bb4..ca23030eff61 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -243,8 +243,18 @@ impl ComponentUiRegistry { let component_query_result = match component_query_result.resolved(origin_db.resolver()) { re_query::PromiseResult::Pending => { - ui.label("Loading component data..."); - return; + // This can currently also happen when there's no data at all. + if component_query_result.num_instances() == 0 { + None + } else { + // This should be possible right now and is an error. + //ui.label("Loading data..."); + ctx.viewer_ctx.re_ui.error_label_and_log_once( + ui, + format!("Promise for {component_name} is still pending."), + ); + return; + } } re_query::PromiseResult::Ready(cell) => { let index = instance.get(); @@ -255,10 +265,10 @@ impl ComponentUiRegistry { } } re_query::PromiseResult::Error(err) => { - let error_text = re_error::format_ref(err.as_ref()); - re_log::error_once!("Couldn't get {component_name}: {error_text}"); - ui.label(ctx.viewer_ctx.re_ui.error_text("Error")) - .on_hover_text(error_text); + ctx.viewer_ctx.re_ui.error_label_and_log_once( + ui, + format!("Couldn't get {component_name}: {err}"), + ); return; } }; @@ -269,10 +279,10 @@ impl ComponentUiRegistry { match fallback_provider.fallback_for(ctx, component_name) { Ok(fallback) => fallback, Err(ComponentFallbackError::MissingBaseFallback) => { - let error = format!("No fallback value available for {component_name}."); - re_log::error_once!("{error}"); - ui.label(ctx.viewer_ctx.re_ui.error_text("")) - .on_hover_text(error); + ctx.viewer_ctx.re_ui.error_label_and_log_once( + ui, + format!("No fallback value available for {component_name}."), + ); return; } } From 8f23d5606f18190b38f68ee36ef59eafc9c63ba1 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:00:54 +0200 Subject: [PATCH 11/24] remove dead code flag from list_default_components --- crates/re_types_builder/src/codegen/rust/api.rs | 1 - crates/re_viewer/src/component_defaults/mod.rs | 2 -- 2 files changed, 3 deletions(-) diff --git a/crates/re_types_builder/src/codegen/rust/api.rs b/crates/re_types_builder/src/codegen/rust/api.rs index b842e68d02c8..621b28c5f219 100644 --- a/crates/re_types_builder/src/codegen/rust/api.rs +++ b/crates/re_types_builder/src/codegen/rust/api.rs @@ -313,7 +313,6 @@ fn generate_component_defaults( use re_types_core::{external::arrow2, ComponentName, SerializationError}; #docs - #[allow(dead_code)] // TODO(#6434): Temporary, working on the user. pub fn list_default_components() -> Result)>, SerializationError> { use ::re_types_core::{Loggable, LoggableBatch as _}; diff --git a/crates/re_viewer/src/component_defaults/mod.rs b/crates/re_viewer/src/component_defaults/mod.rs index 30dbc98f475e..e53a5cc6f1e4 100644 --- a/crates/re_viewer/src/component_defaults/mod.rs +++ b/crates/re_viewer/src/component_defaults/mod.rs @@ -9,8 +9,6 @@ use re_types_core::components::*; use re_types_core::{external::arrow2, ComponentName, SerializationError}; /// Calls `default` for each component type in this module and serializes it to arrow. This is useful as a base fallback value when displaying ui. -#[allow(dead_code)] - pub fn list_default_components( ) -> Result)>, SerializationError> { From f95a37243bbc6c2916a5b3bdac764f4165fa2fb7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:19:24 +0200 Subject: [PATCH 12/24] code cleanup, comment improvements --- .../src/component_ui_registry.rs | 82 ++++++++++--------- crates/re_viewer_context/src/query_context.rs | 3 +- 2 files changed, 44 insertions(+), 41 deletions(-) diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index ca23030eff61..f86787635760 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -4,7 +4,10 @@ use re_data_store::LatestAtQuery; use re_entity_db::{external::re_query::LatestAtComponentResults, EntityDb, EntityPath}; use re_log::ResultExt; use re_log_types::Instance; -use re_types::{external::arrow2, ComponentName}; +use re_types::{ + external::arrow2::{self}, + ComponentName, +}; use crate::{ComponentFallbackError, ComponentFallbackProvider, QueryContext, ViewerContext}; @@ -94,10 +97,8 @@ impl ComponentUiRegistry { /// Registers how to edit a given component in the ui. /// - /// Requires two callbacks: one to provide an initial default value, and one to show the editor - /// UI and save the updated value. - /// /// If the component was already registered, the new callback replaces the old one. + /// Prefer [`ComponentUiRegistry::add_editor`] whenever possible pub fn add_untyped_editor( &mut self, name: ComponentName, @@ -121,8 +122,8 @@ impl ComponentUiRegistry { /// * Don't show a tooltip, this is solved at a higher level. /// * Try not to assume context of the component beyond its inherent semantics /// (e.g. if you get a `Color` you can't assume whether it's a background color or a point color) - /// - /// TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2d bounds is too complex for a single line). + // + // TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2d bounds is too complex for a single line). pub fn add_editor( &mut self, editor_callback: impl Fn(&ViewerContext<'_>, &mut egui::Ui, &mut C) -> egui::Response @@ -130,46 +131,47 @@ impl ComponentUiRegistry { + Sync + 'static, ) { - let untyped_callback: UntypedComponentEditCallback = - Box::new(move |ui, ui_layout, value| { - let deserialized = C::from_arrow(value); - let mut deserialized_value = match deserialized { - Ok(values) => { - if values.len() > 1 { - // Whatever we did prior to calling this should have taken care if it! - re_log::error_once!( - "Can only edit a single value at a time, got {} values for editing {}", + fn try_deserialize(value: &dyn arrow2::array::Array) -> Option { + let component_name = C::name(); + let deserialized = C::from_arrow(value); + match deserialized { + Ok(values) => { + if values.len() > 1 { + // Whatever we did prior to calling this should have taken care if it! + re_log::error_once!( + "Can only edit a single value at a time, got {} values for editing {component_name}", values.len(), - C::name() ); - } - if let Some(v) = values.into_iter().next() { - v - } else { - re_log::warn_once!( - "Editor ui for {} needs a start value to operate on.", - C::name() - ); - return None; - } } - Err(err) => { - re_log::error_once!( - "Failed to deserialize component of type {}: {:?}", - C::name(), - err + if let Some(v) = values.into_iter().next() { + Some(v) + } else { + re_log::warn_once!( + "Editor ui for {component_name} needs a start value to operate on." ); - return None; + None } - }; + } + Err(err) => { + re_log::error_once!( + "Failed to deserialize component of type {component_name}: {err}", + ); + None + } + } + } - editor_callback(ui, ui_layout, &mut deserialized_value) - .changed() - .then(|| { - use re_types::LoggableBatch; - deserialized_value.to_arrow().ok_or_log_error_once() - }) - .flatten() + let untyped_callback: UntypedComponentEditCallback = + Box::new(move |ui, ui_layout, value| { + try_deserialize(value).and_then(|mut deserialized_value| { + editor_callback(ui, ui_layout, &mut deserialized_value) + .changed() + .then(|| { + use re_types::LoggableBatch; + deserialized_value.to_arrow().ok_or_log_error_once() + }) + .flatten() + }) }); self.add_untyped_editor(C::name(), untyped_callback); diff --git a/crates/re_viewer_context/src/query_context.rs b/crates/re_viewer_context/src/query_context.rs index 0cde9a4f359b..278d94fbb12a 100644 --- a/crates/re_viewer_context/src/query_context.rs +++ b/crates/re_viewer_context/src/query_context.rs @@ -13,8 +13,9 @@ slotmap::new_key_type! { } /// Context for a latest at query in a specific view. -/// // TODO(andreas) this is centered around latest-at queries. Does it have to be? Makes sense for UI, but that means it won't scale much into Visualizer queriers. +// This is currently used only for fallback providers, but the expectation is that we're using this more widely as the primary context object +// in all places where we query a specific entity in a specific view. pub struct QueryContext<'a> { pub viewer_ctx: &'a ViewerContext<'a>, From 7c79ecc971ca14c9dbef482b304d20b0fbedc39d Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:32:54 +0200 Subject: [PATCH 13/24] simplify component ui registry impl a bit --- crates/re_ui/src/lib.rs | 11 --- .../src/component_ui_registry.rs | 99 ++++++++++--------- 2 files changed, 54 insertions(+), 56 deletions(-) diff --git a/crates/re_ui/src/lib.rs b/crates/re_ui/src/lib.rs index 41ec2d589678..09dd58679520 100644 --- a/crates/re_ui/src/lib.rs +++ b/crates/re_ui/src/lib.rs @@ -281,17 +281,6 @@ impl ReUi { .on_hover_text(error_text.into()) } - /// Shows a small error label with the given text on hover and logs the error once to the console. - pub fn error_label_and_log_once( - &self, - ui: &mut egui::Ui, - error_text: impl Into, - ) -> egui::Response { - let error_text = error_text.into(); - re_log::error_once!("{error_text}"); - self.error_label(ui, error_text) - } - /// The color we use to mean "loop this selection" pub fn loop_selection_color() -> egui::Color32 { egui::Color32::from_rgb(1, 37, 105) // from figma 2023-02-09 diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index f86787635760..90d09a9ad662 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -9,7 +9,7 @@ use re_types::{ ComponentName, }; -use crate::{ComponentFallbackError, ComponentFallbackProvider, QueryContext, ViewerContext}; +use crate::{ComponentFallbackProvider, QueryContext, ViewerContext}; /// Specifies the context in which the UI is used and the constraints it should follow. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -242,54 +242,22 @@ impl ComponentUiRegistry { let instance: Instance = 0.into(); if let Some(edit_callback) = self.component_editors.get(&component_name) { - let component_query_result = match component_query_result.resolved(origin_db.resolver()) - { - re_query::PromiseResult::Pending => { - // This can currently also happen when there's no data at all. - if component_query_result.num_instances() == 0 { - None - } else { - // This should be possible right now and is an error. - //ui.label("Loading data..."); - ctx.viewer_ctx.re_ui.error_label_and_log_once( - ui, - format!("Promise for {component_name} is still pending."), - ); - return; - } - } - re_query::PromiseResult::Ready(cell) => { - let index = instance.get(); - if cell.num_instances() > index as u32 { - Some(cell.as_arrow_ref().sliced(index as usize, 1)) - } else { - None - } - } - re_query::PromiseResult::Error(err) => { - ctx.viewer_ctx.re_ui.error_label_and_log_once( - ui, - format!("Couldn't get {component_name}: {err}"), - ); + let component_value_or_fallback = match component_value_or_fallback( + ctx, + component_query_result, + component_name, + instance, + origin_db.resolver(), + fallback_provider, + ) { + Ok(value) => value, + Err(error_text) => { + re_log::error_once!("{error_text}"); + ctx.viewer_ctx.re_ui.error_label(ui, error_text); return; } }; - let component_value_or_fallback = if let Some(result) = component_query_result { - result - } else { - match fallback_provider.fallback_for(ctx, component_name) { - Ok(fallback) => fallback, - Err(ComponentFallbackError::MissingBaseFallback) => { - ctx.viewer_ctx.re_ui.error_label_and_log_once( - ui, - format!("No fallback value available for {component_name}."), - ); - return; - } - } - }; - if let Some(updated) = (*edit_callback)(ctx.viewer_ctx, ui, component_value_or_fallback.as_ref()) { @@ -313,3 +281,44 @@ impl ComponentUiRegistry { } } } + +fn component_value_or_fallback( + ctx: &QueryContext<'_>, + component_query_result: &LatestAtComponentResults, + component_name: ComponentName, + instance: Instance, + resolver: &re_query::PromiseResolver, + fallback_provider: &dyn ComponentFallbackProvider, +) -> Result, String> { + match component_query_result.resolved(resolver) { + re_query::PromiseResult::Pending => { + // This can currently also happen when there's no data at all. + if component_query_result.num_instances() == 0 { + None + } else { + // This should be possible right now and is an error. + //ui.label("Loading data..."); + return Err(format!("Promise for {component_name} is still pending.")); + } + } + re_query::PromiseResult::Ready(cell) => { + let index = instance.get(); + if cell.num_instances() > index as u32 { + Some(cell.as_arrow_ref().sliced(index as usize, 1)) + } else { + None + } + } + re_query::PromiseResult::Error(err) => { + return Err(format!("Couldn't get {component_name}: {err}")); + } + } + .map_or_else( + || { + fallback_provider + .fallback_for(ctx, component_name) + .map_err(|_err| format!("No fallback value available for {component_name}.")) + }, + Ok, + ) +} From 463d6545ebdaf6b94375e767269dd2c39348cd1e Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:34:49 +0200 Subject: [PATCH 14/24] rename initial_override_color to fallback_color --- .../re_space_view_time_series/src/line_visualizer_system.rs | 4 ++-- crates/re_space_view_time_series/src/overrides.rs | 2 +- .../re_space_view_time_series/src/point_visualizer_system.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/re_space_view_time_series/src/line_visualizer_system.rs b/crates/re_space_view_time_series/src/line_visualizer_system.rs index e11a564bd287..e542c8ea7d5f 100644 --- a/crates/re_space_view_time_series/src/line_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/line_visualizer_system.rs @@ -12,7 +12,7 @@ use re_viewer_context::{ VisualizerSystem, }; -use crate::overrides::initial_override_color; +use crate::overrides::fallback_color; use crate::util::{ determine_plot_bounds_and_time_per_pixel, determine_time_range, points_to_series, }; @@ -78,7 +78,7 @@ impl VisualizerSystem for SeriesLineSystem { impl TypedComponentFallbackProvider for SeriesLineSystem { fn fallback_for(&self, ctx: &QueryContext<'_>) -> Color { - initial_override_color(ctx.target_entity_path) + fallback_color(ctx.target_entity_path) } } diff --git a/crates/re_space_view_time_series/src/overrides.rs b/crates/re_space_view_time_series/src/overrides.rs index d2f0afc9e8fd..fc81d5b1e845 100644 --- a/crates/re_space_view_time_series/src/overrides.rs +++ b/crates/re_space_view_time_series/src/overrides.rs @@ -2,7 +2,7 @@ use re_log_types::EntityPath; use re_types::components::Color; use re_viewer_context::{DefaultColor, ResolvedAnnotationInfo}; -pub fn initial_override_color(entity_path: &EntityPath) -> Color { +pub fn fallback_color(entity_path: &EntityPath) -> Color { let default_color = DefaultColor::EntityPath(entity_path); let annotation_info = ResolvedAnnotationInfo::default(); diff --git a/crates/re_space_view_time_series/src/point_visualizer_system.rs b/crates/re_space_view_time_series/src/point_visualizer_system.rs index 946c9fbde25d..0a421f08ab14 100644 --- a/crates/re_space_view_time_series/src/point_visualizer_system.rs +++ b/crates/re_space_view_time_series/src/point_visualizer_system.rs @@ -12,7 +12,7 @@ use re_viewer_context::{ VisualizerSystem, }; -use crate::overrides::initial_override_color; +use crate::overrides::fallback_color; use crate::util::{ determine_plot_bounds_and_time_per_pixel, determine_time_range, points_to_series, }; @@ -81,7 +81,7 @@ impl VisualizerSystem for SeriesPointSystem { impl TypedComponentFallbackProvider for SeriesPointSystem { fn fallback_for(&self, ctx: &QueryContext<'_>) -> Color { - initial_override_color(ctx.target_entity_path) + fallback_color(ctx.target_entity_path) } } From 380dcb359ab940fb20bc781f90d613d327f2d879 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:37:11 +0200 Subject: [PATCH 15/24] lint fixes --- crates/re_viewer_context/src/component_fallbacks.rs | 2 +- crates/re_viewer_context/src/component_ui_registry.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/re_viewer_context/src/component_fallbacks.rs b/crates/re_viewer_context/src/component_fallbacks.rs index cb89490ff475..77f50ca16183 100644 --- a/crates/re_viewer_context/src/component_fallbacks.rs +++ b/crates/re_viewer_context/src/component_fallbacks.rs @@ -70,7 +70,7 @@ pub trait ComponentFallbackProvider { // but arrow serialization should never fail. // Giving out _both_ the error and the fallback value gets messy, // so given that this should be a rare bug, we log it and return the fallback value as success. - re_log::error_once!("Arrow serialization failed trying to provide a fallback for {:?}. Using base fallback instead: {}", component, err); + re_log::error_once!("Arrow serialization failed trying to provide a fallback for {component}. Using base fallback instead: {err}"); } ComponentFallbackProviderResult::ComponentNotHandled => {} } diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index 90d09a9ad662..d0de67bf9e79 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -123,7 +123,7 @@ impl ComponentUiRegistry { /// * Try not to assume context of the component beyond its inherent semantics /// (e.g. if you get a `Color` you can't assume whether it's a background color or a point color) // - // TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2d bounds is too complex for a single line). + // TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2D bounds is too complex for a single line). pub fn add_editor( &mut self, editor_callback: impl Fn(&ViewerContext<'_>, &mut egui::Ui, &mut C) -> egui::Response @@ -292,8 +292,8 @@ fn component_value_or_fallback( ) -> Result, String> { match component_query_result.resolved(resolver) { re_query::PromiseResult::Pending => { - // This can currently also happen when there's no data at all. if component_query_result.num_instances() == 0 { + // This can currently also happen when there's no data at all. None } else { // This should be possible right now and is an error. From 04e9a39ffb8dea7d4da654dc512d176a4a377fb6 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 May 2024 17:37:53 +0200 Subject: [PATCH 16/24] fix typo --- crates/re_space_view_time_series/src/space_view_class.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_space_view_time_series/src/space_view_class.rs b/crates/re_space_view_time_series/src/space_view_class.rs index 3670d2d045fb..42add23e86e8 100644 --- a/crates/re_space_view_time_series/src/space_view_class.rs +++ b/crates/re_space_view_time_series/src/space_view_class.rs @@ -806,7 +806,7 @@ fn round_ns_to_start_of_day(ns: i64) -> i64 { impl TypedComponentFallbackProvider for TimeSeriesSpaceView { fn fallback_for(&self, _ctx: &re_viewer_context::QueryContext<'_>) -> Corner2D { - // Explicitely pick RightCorner2D::RightBottom, we don't want to make this dependent on the (arbitrary) + // Explicitly pick RightCorner2D::RightBottom, we don't want to make this dependent on the (arbitrary) // default of Corner2D Corner2D::RightBottom } From d705704c9224be7869c4add9639b5dfbdc2da172 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 09:25:52 +0200 Subject: [PATCH 17/24] use `|` instead of union for concatting responses --- crates/re_edit_ui/src/corner2d.rs | 11 ++++------- crates/re_edit_ui/src/lib.rs | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/re_edit_ui/src/corner2d.rs b/crates/re_edit_ui/src/corner2d.rs index 7d91bfebef5f..b0741f0a61cf 100644 --- a/crates/re_edit_ui/src/corner2d.rs +++ b/crates/re_edit_ui/src/corner2d.rs @@ -13,22 +13,19 @@ pub fn edit_corner2d( value, egui_plot::Corner::LeftTop.into(), format!("{}", Corner2D::from(egui_plot::Corner::LeftTop)), - ) - .union(ui.selectable_value( + ) | ui.selectable_value( value, egui_plot::Corner::RightTop.into(), format!("{}", Corner2D::from(egui_plot::Corner::RightTop)), - )) - .union(ui.selectable_value( + ) | ui.selectable_value( value, egui_plot::Corner::LeftBottom.into(), format!("{}", Corner2D::from(egui_plot::Corner::LeftBottom)), - )) - .union(ui.selectable_value( + ) | ui.selectable_value( value, egui_plot::Corner::RightBottom.into(), format!("{}", Corner2D::from(egui_plot::Corner::RightBottom)), - )) + ) }); outer_response.inner.unwrap_or(outer_response.response) diff --git a/crates/re_edit_ui/src/lib.rs b/crates/re_edit_ui/src/lib.rs index 3a383c03309c..9064943dafe8 100644 --- a/crates/re_edit_ui/src/lib.rs +++ b/crates/re_edit_ui/src/lib.rs @@ -52,7 +52,7 @@ fn edit_scatter_ui( .show_ui(ui, |ui| { ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); ui.selectable_value(&mut value.0, false, "Line") - .union(ui.selectable_value(&mut value.0, true, "Scattered")) + | ui.selectable_value(&mut value.0, true, "Scattered") }); outer_response.inner.unwrap_or(outer_response.response) From 74a80f2395beec46fcc1472988da28646fcfdf33 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:17:48 +0200 Subject: [PATCH 18/24] remove some no longer needed ui calls --- crates/re_edit_ui/src/lib.rs | 1 - crates/re_edit_ui/src/marker_shape.rs | 9 --------- 2 files changed, 10 deletions(-) diff --git a/crates/re_edit_ui/src/lib.rs b/crates/re_edit_ui/src/lib.rs index 9064943dafe8..9fd44c233bc6 100644 --- a/crates/re_edit_ui/src/lib.rs +++ b/crates/re_edit_ui/src/lib.rs @@ -50,7 +50,6 @@ fn edit_scatter_ui( let outer_response = egui::ComboBox::from_id_source("scatter") .selected_text(scattered_text) .show_ui(ui, |ui| { - ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); ui.selectable_value(&mut value.0, false, "Line") | ui.selectable_value(&mut value.0, true, "Scattered") }); diff --git a/crates/re_edit_ui/src/marker_shape.rs b/crates/re_edit_ui/src/marker_shape.rs index 8b625dbc2bb1..cbb69a87cfc7 100644 --- a/crates/re_edit_ui/src/marker_shape.rs +++ b/crates/re_edit_ui/src/marker_shape.rs @@ -1,5 +1,3 @@ -use egui::NumExt as _; - use re_types::components::MarkerShape; use re_viewer_context::ViewerContext; @@ -14,15 +12,8 @@ pub(crate) fn edit_marker_shape_ui( let outer_response = egui::ComboBox::from_id_source("marker_shape") .selected_text(marker_text) - .width( - ui.available_width() - .at_most(item_width + ui.spacing().menu_margin.sum().x), - ) .height(320.0) .show_ui(ui, |ui| { - // workaround to force `ui.max_rect()` to reflect the content size - ui.set_width(item_width); - let background_x_range = (ui.max_rect() + ui.spacing().menu_margin).x_range(); let list_ui = |ui: &mut egui::Ui| { From 37976d709e5e980e47fe0b7b9c95e26bf291096b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:23:53 +0200 Subject: [PATCH 19/24] copy text on click on error label --- crates/re_ui/src/lib.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/re_ui/src/lib.rs b/crates/re_ui/src/lib.rs index 09dd58679520..c710cba43d8c 100644 --- a/crates/re_ui/src/lib.rs +++ b/crates/re_ui/src/lib.rs @@ -275,10 +275,16 @@ impl ReUi { .color(style.visuals.error_fg_color) } - /// Shows a small error label with the given text on hover. - pub fn error_label(&self, ui: &mut egui::Ui, error_text: impl Into) -> egui::Response { - ui.label(self.error_text("Error")) - .on_hover_text(error_text.into()) + /// Shows a small error label with the given text on hover and copies the text to the clipboard on click. + pub fn error_label(&self, ui: &mut egui::Ui, error_text: &str) -> egui::Response { + let label = egui::Label::new(self.error_text("Error")) + .selectable(false) + .sense(egui::Sense::click()); + let response = ui.add(label); + if response.clicked() { + ui.ctx().copy_text(error_text.to_owned()); + } + response.on_hover_text(error_text) } /// The color we use to mean "loop this selection" From 8e5687685efac54ddc2f493404237dce58d95fd4 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:25:27 +0200 Subject: [PATCH 20/24] document MissingBaseFallback error --- crates/re_viewer_context/src/component_fallbacks.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/re_viewer_context/src/component_fallbacks.rs b/crates/re_viewer_context/src/component_fallbacks.rs index 77f50ca16183..543959a6a561 100644 --- a/crates/re_viewer_context/src/component_fallbacks.rs +++ b/crates/re_viewer_context/src/component_fallbacks.rs @@ -34,7 +34,10 @@ impl From for ComponentFallbackProviderResult { /// Result for a fallback request. pub enum ComponentFallbackError { /// The fallback provider is not able to handle the given component _and_ there was no placeholder value. - /// This should never happen, since all components should have a placeholder value. + /// + /// This should never happen, since all components should have a placeholder value + /// registered in [`crate::ViewerContext::component_placeholders`]. + /// Meaning, that this is an unknown component or something went wrong with the placeholder registration. MissingBaseFallback, } From 611e822174d093a09a88cdc7a05b61dbd9e9b1e3 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:26:14 +0200 Subject: [PATCH 21/24] fix confusing/wrong comment --- crates/re_viewer_context/src/component_ui_registry.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index d0de67bf9e79..b9780e2eef3c 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -296,8 +296,8 @@ fn component_value_or_fallback( // This can currently also happen when there's no data at all. None } else { - // This should be possible right now and is an error. - //ui.label("Loading data..."); + // In the future, we might want to show a loading indicator here, + // but right now this is always an error. return Err(format!("Promise for {component_name} is still pending.")); } } From a3def7ecb406ae3d8ef200c3e25563036fdcaed0 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:37:11 +0200 Subject: [PATCH 22/24] typo --- crates/re_viewer_context/src/component_ui_registry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index b9780e2eef3c..bfc3d435a046 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -253,7 +253,7 @@ impl ComponentUiRegistry { Ok(value) => value, Err(error_text) => { re_log::error_once!("{error_text}"); - ctx.viewer_ctx.re_ui.error_label(ui, error_text); + ctx.viewer_ctx.re_ui.error_label(ui, &error_text); return; } }; From 888d3bd6eabd1216966b56bfd7cef15f7a8c6740 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 10:37:22 +0200 Subject: [PATCH 23/24] another response combine --- crates/re_edit_ui/src/marker_shape.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_edit_ui/src/marker_shape.rs b/crates/re_edit_ui/src/marker_shape.rs index cbb69a87cfc7..fb0c25b0dec8 100644 --- a/crates/re_edit_ui/src/marker_shape.rs +++ b/crates/re_edit_ui/src/marker_shape.rs @@ -38,7 +38,7 @@ pub(crate) fn edit_marker_shape_ui( } combined_response = Some(match combined_response { - Some(combined_response) => combined_response.union(response), + Some(combined_response) => combined_response | response, None => response, }); } From 73836bcfb32d044921890c59b1ce89d4dd9abf3a Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 31 May 2024 11:12:01 +0200 Subject: [PATCH 24/24] use outer reponse with change mark from inner response --- crates/re_edit_ui/src/corner2d.rs | 48 ++++++------ crates/re_edit_ui/src/lib.rs | 3 + crates/re_edit_ui/src/marker_shape.rs | 76 ++++++++++--------- crates/re_edit_ui/src/response_utils.rs | 11 +++ .../src/component_ui_registry.rs | 2 + 5 files changed, 80 insertions(+), 60 deletions(-) create mode 100644 crates/re_edit_ui/src/response_utils.rs diff --git a/crates/re_edit_ui/src/corner2d.rs b/crates/re_edit_ui/src/corner2d.rs index b0741f0a61cf..4079e3166003 100644 --- a/crates/re_edit_ui/src/corner2d.rs +++ b/crates/re_edit_ui/src/corner2d.rs @@ -1,32 +1,34 @@ use re_types::blueprint::components::Corner2D; use re_viewer_context::ViewerContext; +use crate::response_utils::response_with_changes_of_inner; + pub fn edit_corner2d( _ctx: &ViewerContext<'_>, ui: &mut egui::Ui, value: &mut Corner2D, ) -> egui::Response { - let outer_response = egui::ComboBox::from_id_source("corner2d") - .selected_text(format!("{value}")) - .show_ui(ui, |ui| { - ui.selectable_value( - value, - egui_plot::Corner::LeftTop.into(), - format!("{}", Corner2D::from(egui_plot::Corner::LeftTop)), - ) | ui.selectable_value( - value, - egui_plot::Corner::RightTop.into(), - format!("{}", Corner2D::from(egui_plot::Corner::RightTop)), - ) | ui.selectable_value( - value, - egui_plot::Corner::LeftBottom.into(), - format!("{}", Corner2D::from(egui_plot::Corner::LeftBottom)), - ) | ui.selectable_value( - value, - egui_plot::Corner::RightBottom.into(), - format!("{}", Corner2D::from(egui_plot::Corner::RightBottom)), - ) - }); - - outer_response.inner.unwrap_or(outer_response.response) + response_with_changes_of_inner( + egui::ComboBox::from_id_source("corner2d") + .selected_text(format!("{value}")) + .show_ui(ui, |ui| { + ui.selectable_value( + value, + egui_plot::Corner::LeftTop.into(), + format!("{}", Corner2D::from(egui_plot::Corner::LeftTop)), + ) | ui.selectable_value( + value, + egui_plot::Corner::RightTop.into(), + format!("{}", Corner2D::from(egui_plot::Corner::RightTop)), + ) | ui.selectable_value( + value, + egui_plot::Corner::LeftBottom.into(), + format!("{}", Corner2D::from(egui_plot::Corner::LeftBottom)), + ) | ui.selectable_value( + value, + egui_plot::Corner::RightBottom.into(), + format!("{}", Corner2D::from(egui_plot::Corner::RightBottom)), + ) + }), + ) } diff --git a/crates/re_edit_ui/src/lib.rs b/crates/re_edit_ui/src/lib.rs index 9fd44c233bc6..30d731b30f49 100644 --- a/crates/re_edit_ui/src/lib.rs +++ b/crates/re_edit_ui/src/lib.rs @@ -5,8 +5,11 @@ mod corner2d; mod marker_shape; +mod response_utils; mod visible; +// ---- + use egui::NumExt as _; use re_types::components::{Color, MarkerSize, Name, Radius, ScalarScattering, StrokeWidth, Text}; use re_viewer_context::ViewerContext; diff --git a/crates/re_edit_ui/src/marker_shape.rs b/crates/re_edit_ui/src/marker_shape.rs index fb0c25b0dec8..761cd8541ac0 100644 --- a/crates/re_edit_ui/src/marker_shape.rs +++ b/crates/re_edit_ui/src/marker_shape.rs @@ -1,6 +1,8 @@ use re_types::components::MarkerShape; use re_viewer_context::ViewerContext; +use crate::response_utils::response_with_changes_of_inner; + pub(crate) fn edit_marker_shape_ui( ctx: &ViewerContext<'_>, ui: &mut egui::Ui, @@ -10,47 +12,47 @@ pub(crate) fn edit_marker_shape_ui( let item_width = 100.0; - let outer_response = egui::ComboBox::from_id_source("marker_shape") - .selected_text(marker_text) - .height(320.0) - .show_ui(ui, |ui| { - let background_x_range = (ui.max_rect() + ui.spacing().menu_margin).x_range(); - - let list_ui = |ui: &mut egui::Ui| { - let mut combined_response: Option = None; - for marker in MarkerShape::ALL { - let mut response = ctx - .re_ui - .list_item() - .selected(*edit_marker == marker) - .show_flat( - ui, - re_ui::list_item::LabelContent::new(marker.to_string()) - .min_desired_width(item_width) - .with_icon_fn(|_re_ui, ui, rect, visuals| { - paint_marker(ui, marker.into(), rect, visuals.text_color()); - }), - ); + response_with_changes_of_inner( + egui::ComboBox::from_id_source("marker_shape") + .selected_text(marker_text) + .height(320.0) + .show_ui(ui, |ui| { + let background_x_range = (ui.max_rect() + ui.spacing().menu_margin).x_range(); - if response.clicked() { - *edit_marker = marker; - response.changed = true; - } + let list_ui = |ui: &mut egui::Ui| { + let mut combined_response: Option = None; + for marker in MarkerShape::ALL { + let mut response = ctx + .re_ui + .list_item() + .selected(*edit_marker == marker) + .show_flat( + ui, + re_ui::list_item::LabelContent::new(marker.to_string()) + .min_desired_width(item_width) + .with_icon_fn(|_re_ui, ui, rect, visuals| { + paint_marker(ui, marker.into(), rect, visuals.text_color()); + }), + ); - combined_response = Some(match combined_response { - Some(combined_response) => combined_response | response, - None => response, - }); - } - combined_response.expect("At least one marker shape should be available") - }; + if response.clicked() { + *edit_marker = marker; + response.changed = true; + } - re_ui::full_span::full_span_scope(ui, background_x_range, |ui| { - re_ui::list_item::list_item_scope(ui, "marker_shape", list_ui) - }) - }); + combined_response = Some(match combined_response { + Some(combined_response) => combined_response | response, + None => response, + }); + } + combined_response.expect("At least one marker shape should be available") + }; - outer_response.inner.unwrap_or(outer_response.response) + re_ui::full_span::full_span_scope(ui, background_x_range, |ui| { + re_ui::list_item::list_item_scope(ui, "marker_shape", list_ui) + }) + }), + ) } pub(crate) fn paint_marker( diff --git a/crates/re_edit_ui/src/response_utils.rs b/crates/re_edit_ui/src/response_utils.rs new file mode 100644 index 000000000000..60e7e8dcd65c --- /dev/null +++ b/crates/re_edit_ui/src/response_utils.rs @@ -0,0 +1,11 @@ +/// Forwards the changed state from the inner response to the outer response and returns it. +pub fn response_with_changes_of_inner( + mut inner_response: egui::InnerResponse>, +) -> egui::Response { + if let Some(inner) = inner_response.inner { + if inner.changed() { + inner_response.response.mark_changed(); + } + } + inner_response.response +} diff --git a/crates/re_viewer_context/src/component_ui_registry.rs b/crates/re_viewer_context/src/component_ui_registry.rs index bfc3d435a046..a7a001f30303 100644 --- a/crates/re_viewer_context/src/component_ui_registry.rs +++ b/crates/re_viewer_context/src/component_ui_registry.rs @@ -122,6 +122,8 @@ impl ComponentUiRegistry { /// * Don't show a tooltip, this is solved at a higher level. /// * Try not to assume context of the component beyond its inherent semantics /// (e.g. if you get a `Color` you can't assume whether it's a background color or a point color) + /// * The returned [`egui::Response`] should be for the widget that has the tooltip, not any pop-up content. + /// * Make sure that changes are propagated via [`egui::Response::mark_changed`] if necessary. // // TODO(andreas): Implement handling for ui elements that are expandable (e.g. 2D bounds is too complex for a single line). pub fn add_editor(