From 327791b1b9e603eccbb19d1f25dba7f7c25066c5 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 11 Sep 2024 14:45:18 +0200 Subject: [PATCH 01/19] First working prototype --- Cargo.lock | 2 +- .../viewer/re_space_view_dataframe/Cargo.toml | 2 +- .../src/dataframe_ui.rs | 86 ++++++++++++++- .../src/display_record_batch.rs | 88 +++++++++++++-- .../src/expanded_rows.rs | 102 ++++++++++++++++++ .../viewer/re_space_view_dataframe/src/lib.rs | 1 + .../src/space_view_class.rs | 39 +++++-- crates/viewer/re_ui/src/design_tokens.rs | 2 +- .../instance_count_test.py | 55 ++++++++++ 9 files changed, 356 insertions(+), 21 deletions(-) create mode 100644 crates/viewer/re_space_view_dataframe/src/expanded_rows.rs create mode 100644 tests/python/instance_count_test/instance_count_test.py diff --git a/Cargo.lock b/Cargo.lock index ccf83cfd8887..8f499981fb6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1947,7 +1947,7 @@ dependencies = [ [[package]] name = "egui_table" version = "0.28.1" -source = "git+https://github.com/rerun-io/egui_table.git?rev=0f594701d528c4a9553521cb941de1886549dc70#0f594701d528c4a9553521cb941de1886549dc70" +source = "git+https://github.com/rerun-io/egui_table.git?rev=36887f386bdd5a324c0ae06f1cae5d73524c9b77#36887f386bdd5a324c0ae06f1cae5d73524c9b77" dependencies = [ "egui", "serde", diff --git a/crates/viewer/re_space_view_dataframe/Cargo.toml b/crates/viewer/re_space_view_dataframe/Cargo.toml index a92540e3bb41..537bfc7c96e1 100644 --- a/crates/viewer/re_space_view_dataframe/Cargo.toml +++ b/crates/viewer/re_space_view_dataframe/Cargo.toml @@ -36,7 +36,7 @@ re_viewport_blueprint.workspace = true anyhow.workspace = true egui_extras.workspace = true -egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "0f594701d528c4a9553521cb941de1886549dc70" } # main as of 2024-09-09 +egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "36887f386bdd5a324c0ae06f1cae5d73524c9b77" } # PR #14 as of 2024-09-11 egui.workspace = true itertools.workspace = true thiserror.workspace = true diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 0ded69c81d30..0719875de9a8 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -13,14 +13,16 @@ use re_ui::UiExt as _; use re_viewer_context::ViewerContext; use crate::display_record_batch::{DisplayRecordBatch, DisplayRecordBatchError}; +use crate::expanded_rows::{ExpandedRows, ExpandedRowsCache, QueryExpression}; /// Display a dataframe table for the provided query. pub(crate) fn dataframe_ui<'a>( ctx: &ViewerContext<'_>, ui: &mut egui::Ui, query: impl Into>, + expanded_rows_cache: &mut ExpandedRowsCache, ) { - dataframe_ui_impl(ctx, ui, &query.into()); + dataframe_ui_impl(ctx, ui, &query.into(), expanded_rows_cache); } /// A query handle for either a latest-at or range query. @@ -62,6 +64,13 @@ impl QueryHandle<'_> { QueryHandle::Range(query_handle) => query_handle.query().timeline, } } + + fn query_expression(&self) -> QueryExpression { + match self { + QueryHandle::LatestAt(query_handle) => query_handle.query().clone().into(), + QueryHandle::Range(query_handle) => query_handle.query().clone().into(), + } + } } impl<'a> From> for QueryHandle<'a> { @@ -165,6 +174,8 @@ struct DataframeTableDelegate<'a> { header_entity_paths: Vec>, display_data: anyhow::Result, + expanded_rows: ExpandedRows<'a>, + num_rows: u64, } @@ -276,7 +287,52 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { } else { ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Truncate); } - column.data_ui(self.ctx, ui, row_id, &latest_at_query, row_idx); + + let instance_count = column.instance_count(row_idx); + let row_expansion = self.expanded_rows.row_expansion(cell.row_nr); + + let instance_indices = std::iter::once(None) + .chain((0..instance_count).map(Option::Some)) + .take(row_expansion as usize + 1); + + for (sub_cell_index, instance_index) in instance_indices.enumerate() { + let sub_cell_rect = egui::Rect::from_min_size( + ui.cursor().min + + egui::vec2( + 0.0, + sub_cell_index as f32 * re_ui::DesignTokens::table_line_height(), + ), + egui::vec2( + ui.available_width(), + re_ui::DesignTokens::table_line_height(), + ), + ); + + let mut sub_cell_ui = + ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); + + if instance_index == None && instance_count > 1 { + if sub_cell_ui + .button(format!("{instance_count} instances")) + .clicked() + { + if instance_count == row_expansion { + self.expanded_rows.expend_row(cell.row_nr, 0); + } else { + self.expanded_rows.expend_row(cell.row_nr, instance_count) + } + } + } else { + column.data_ui( + self.ctx, + &mut sub_cell_ui, + row_id, + &latest_at_query, + row_idx, + instance_index, + ); + } + } } else { error_ui( ui, @@ -289,12 +345,29 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .inner_margin(egui::Margin::symmetric(Self::LEFT_RIGHT_MARGIN, 0.0)) .show(ui, cell_ui); } + + fn row_top_offset(&self, ctx: &egui::Context, table_id_salt: egui::Id, row_nr: u64) -> f32 { + self.expanded_rows + .row_top_offset(ctx, table_id_salt, row_nr) + } + + fn default_row_height(&self) -> f32 { + re_ui::DesignTokens::table_line_height() + } } /// Display the result of a [`QueryHandle`] in a table. -fn dataframe_ui_impl(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, query_handle: &QueryHandle<'_>) { +fn dataframe_ui_impl( + ctx: &ViewerContext<'_>, + ui: &mut egui::Ui, + query_handle: &QueryHandle<'_>, + expanded_rows_cache: &mut ExpandedRowsCache, +) { re_tracing::profile_function!(); + //TODO: actually make that unique! + let id = ui.id().with("__dataframe__"); + let schema = query_handle.schema(); let (header_groups, header_entity_paths) = column_groups_for_entity(schema); @@ -309,6 +382,11 @@ fn dataframe_ui_impl(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, query_handle: & display_data: Err(anyhow::anyhow!( "No row data, `fetch_columns_and_rows` not called." )), + expanded_rows: ExpandedRows::new( + expanded_rows_cache, + query_handle.query_expression(), + re_ui::DesignTokens::table_line_height(), + ), }; let num_sticky_cols = schema @@ -318,6 +396,7 @@ fn dataframe_ui_impl(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, query_handle: & egui::Frame::none().inner_margin(5.0).show(ui, |ui| { egui_table::Table::new() + .id_salt(id) .columns( schema .iter() @@ -337,7 +416,6 @@ fn dataframe_ui_impl(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, query_handle: & egui_table::HeaderRow::new(re_ui::DesignTokens::table_header_height()), ]) .num_rows(num_rows) - .row_height(re_ui::DesignTokens::table_line_height()) .show(ui, &mut table_delegate); }); } diff --git a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs index 98cb31e59e3a..1a0c8cf04f0c 100644 --- a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs +++ b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs @@ -76,6 +76,39 @@ impl ComponentData { } } + /// Returns the number of instances for the given row index. + /// + /// For [`Self::Null`] columns, or for invalid `row_index`, this will return 0. + fn instance_count(&self, row_index: usize) -> u64 { + match self { + Self::Null => 0, + Self::ListArray(list_array) => { + if list_array.is_valid(row_index) { + list_array.value(row_index).len() as u64 + } else { + 0 + } + } + Self::DictionaryArray { dict, values } => { + if dict.is_valid(row_index) { + values.value(dict.key_value(row_index)).len() as u64 + } else { + 0 + } + } + } + } + + /// Display some data from the column. + /// + /// - Argument `row_index` is the row index within the batch column. + /// - Argument `instance_index` is the specific instance within the specified row. If `None`, a + /// summary of all existing instances is displayed. + /// + /// # Panic + /// + /// Panics if `instance_index` is out-of-bound. Use [`Self::instance_count`] to ensure + /// correctness. #[allow(clippy::too_many_arguments)] fn data_ui( &self, @@ -85,7 +118,8 @@ impl ComponentData { latest_at_query: &LatestAtQuery, entity_path: &EntityPath, component_name: ComponentName, - row_index: usize, // index within the batch column + row_index: usize, + instance_index: Option, ) { let data = match self { Self::Null => { @@ -101,6 +135,14 @@ impl ComponentData { }; if let Some(data) = data { + let data_to_display = if let Some(instance_index) = instance_index { + // Panics if the instance index is out of bound. This is checked in + // `DisplayColumn::data_ui`. + data.sliced(instance_index as usize, 1) + } else { + data + }; + ctx.component_ui_registry.ui_raw( ctx, ui, @@ -110,7 +152,7 @@ impl ComponentData { entity_path, component_name, Some(row_id), - &*data, + &*data_to_display, ); } else { ui.label("-"); @@ -201,23 +243,49 @@ impl DisplayColumn { } } + pub(crate) fn instance_count(&self, row_index: usize) -> u64 { + match self { + Self::RowId { .. } | Self::Timeline { .. } => 1, + Self::Component { component_data, .. } => component_data.instance_count(row_index), + } + } + + /// Display some data in the column. + /// + /// - Argument `row_index` is the row index within the batch column. + /// - Argument `instance_index` is the specific instance within the row to display. If `None`, + /// a summary of all instances is displayed. If the instance is out-of-bound (aka greater than + /// [`Self::instance_count`]), nothing is displayed. pub(crate) fn data_ui( &self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, row_id: RowId, latest_at_query: &LatestAtQuery, - index: usize, + row_index: usize, + instance_index: Option, ) { + if let Some(instance_index) = instance_index { + if instance_index >= self.instance_count(row_index) { + // do not display anything for out-of-bound instance index + return; + } + } + match self { Self::RowId { row_id_times, row_id_counters, .. } => { + if instance_index.is_some() { + // we only ever display the row id on the summary line + return; + } + let row_id = RowId::from_u128( - (row_id_times.value(index) as u128) << 64 - | (row_id_counters.value(index) as u128), + (row_id_times.value(row_index) as u128) << 64 + | (row_id_counters.value(row_index) as u128), ); row_id_ui(ctx, ui, &row_id); } @@ -225,7 +293,12 @@ impl DisplayColumn { timeline, time_data, } => { - let timestamp = TimeInt::try_from(time_data.value(index)); + if instance_index.is_some() { + // we only ever display the row id on the summary line + return; + } + + let timestamp = TimeInt::try_from(time_data.value(row_index)); match timestamp { Ok(timestamp) => { ui.label(timeline.typ().format(timestamp, ctx.app_options.time_zone)); @@ -247,7 +320,8 @@ impl DisplayColumn { latest_at_query, entity_path, *component_name, - index, + row_index, + instance_index, ); } } diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs new file mode 100644 index 000000000000..7e7c9baac321 --- /dev/null +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -0,0 +1,102 @@ +use std::collections::BTreeMap; + +use re_chunk_store::{LatestAtQueryExpression, RangeQueryExpression}; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum QueryExpression { + LatestAt(re_chunk_store::LatestAtQueryExpression), + Range(re_chunk_store::RangeQueryExpression), +} + +impl From for QueryExpression { + fn from(value: LatestAtQueryExpression) -> Self { + Self::LatestAt(value) + } +} + +impl From for QueryExpression { + fn from(value: RangeQueryExpression) -> Self { + Self::Range(value) + } +} + +/// Storage for [`ExpandedRows`], which should be persisted across frames. +#[derive(Debug, Clone, Default)] +pub(crate) struct ExpandedRowsCache { + /// Maps "table row number" to "additional expanded rows". + /// + /// When expanded, the base space is still used for the summary, which the additional space is + /// used for instances. + expanded_rows: BTreeMap, + + /// Keep track of the query for which this cache is valid. + // TODO(ab): is there a better invalidation strategy? This doesn't capture the fact that the + // returned data might vary with time (e.g. upon ingestion) + valid_for: Option, +} + +impl ExpandedRowsCache { + /// This sets the query used for cache invalidation. + /// + /// If the query doesn't match the cached one, the state will be reset. + fn set_query(&mut self, query_expression: QueryExpression) { + if Some(&query_expression) != self.valid_for.as_ref() { + self.valid_for = Some(query_expression); + self.expanded_rows = BTreeMap::default(); + } + } +} + +/// Helper to keep track of row expansion. +/// +/// This is a short-lived struct to be created every frame. The persistent state is stored in +/// [`ExpandedRowsCache`]. +pub(crate) struct ExpandedRows<'a> { + /// Base row height. + row_height: f32, + + /// Cache containing the row expanded-ness. + cache: &'a mut ExpandedRowsCache, +} + +impl<'a> ExpandedRows<'a> { + pub(crate) fn new( + cache: &'a mut ExpandedRowsCache, + query_expression: impl Into, + row_height: f32, + ) -> Self { + // validate the cache + cache.set_query(query_expression.into()); + + Self { row_height, cache } + } + + /// Implementation for [`egui_table::Table::row_top_offset`]. + pub(crate) fn row_top_offset( + &self, + ctx: &egui::Context, + id_salt: egui::Id, + row_nr: u64, + ) -> f32 { + self.cache + .expanded_rows + .range(0..row_nr) + .map(|(expanded_row_nr, expanded)| { + let how_expanded = ctx.animate_bool(id_salt.with(expanded_row_nr), *expanded > 0); + how_expanded * *expanded as f32 * self.row_height + }) + .sum::() + + row_nr as f32 * self.row_height + } + + pub(crate) fn expend_row(&mut self, row_nr: u64, additional_row_space: u64) { + self.cache + .expanded_rows + .insert(row_nr, additional_row_space); + } + + /// Return by how much row space this row is expended. + pub(crate) fn row_expansion(&self, row_nr: u64) -> u64 { + self.cache.expanded_rows.get(&row_nr).copied().unwrap_or(0) + } +} diff --git a/crates/viewer/re_space_view_dataframe/src/lib.rs b/crates/viewer/re_space_view_dataframe/src/lib.rs index f7093d3a94dc..e25c7b8ed0c8 100644 --- a/crates/viewer/re_space_view_dataframe/src/lib.rs +++ b/crates/viewer/re_space_view_dataframe/src/lib.rs @@ -4,6 +4,7 @@ mod dataframe_ui; mod display_record_batch; +mod expanded_rows; mod query_kind; mod space_view_class; mod view_query; diff --git a/crates/viewer/re_space_view_dataframe/src/space_view_class.rs b/crates/viewer/re_space_view_dataframe/src/space_view_class.rs index 4679c3bc18bb..c2ebf321cca6 100644 --- a/crates/viewer/re_space_view_dataframe/src/space_view_class.rs +++ b/crates/viewer/re_space_view_dataframe/src/space_view_class.rs @@ -1,18 +1,37 @@ use egui::Ui; +use std::any::Any; -use crate::dataframe_ui::dataframe_ui; -use crate::{query_kind::QueryKind, view_query::Query, visualizer_system::EmptySystem}; use re_log_types::{EntityPath, EntityPathFilter, ResolvedTimeRange}; use re_space_view::view_property_ui; use re_types::blueprint::archetypes; use re_types_core::SpaceViewClassIdentifier; use re_ui::list_item; use re_viewer_context::{ - SpaceViewClass, SpaceViewClassRegistryError, SpaceViewId, SpaceViewState, + SpaceViewClass, SpaceViewClassRegistryError, SpaceViewId, SpaceViewState, SpaceViewStateExt, SpaceViewSystemExecutionError, SystemExecutionOutput, ViewQuery, ViewerContext, }; use re_viewport_blueprint::SpaceViewContents; +use crate::dataframe_ui::dataframe_ui; +use crate::expanded_rows::ExpandedRowsCache; +use crate::{query_kind::QueryKind, view_query::Query, visualizer_system::EmptySystem}; + +#[derive(Default)] +struct DataframeSpaceViewState { + /// Cache for the expanded rows. + expended_rows_cache: ExpandedRowsCache, +} + +impl SpaceViewState for DataframeSpaceViewState { + fn as_any(&self) -> &dyn Any { + self + } + + fn as_any_mut(&mut self) -> &mut dyn Any { + self + } +} + #[derive(Default)] pub struct DataframeSpaceView; @@ -59,7 +78,7 @@ mode sets the default time range to _everything_. You can override this in the s } fn new_state(&self) -> Box { - Box::<()>::default() + Box::::default() } fn preferred_tile_aspect_ratio(&self, _state: &dyn SpaceViewState) -> Option { @@ -112,11 +131,12 @@ mode sets the default time range to _everything_. You can override this in the s &self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - _state: &mut dyn SpaceViewState, + state: &mut dyn SpaceViewState, query: &ViewQuery<'_>, _system_output: SystemExecutionOutput, ) -> Result<(), SpaceViewSystemExecutionError> { re_tracing::profile_function!(); + let state = state.downcast_mut::()?; let view_query = super::view_query::Query::try_from_blueprint(ctx, query.space_view_id)?; let timeline_name = view_query.timeline_name(ctx); @@ -148,7 +168,7 @@ mode sets the default time range to _everything_. You can override this in the s //TODO(ab): specify which columns let query_handle = query_engine.latest_at(&query, None); - dataframe_ui(ctx, ui, query_handle); + dataframe_ui(ctx, ui, query_handle, &mut state.expended_rows_cache); } QueryKind::Range { pov_entity, @@ -173,7 +193,12 @@ mode sets the default time range to _everything_. You can override this in the s }; //TODO(ab): specify which columns should be displayed or not - dataframe_ui(ctx, ui, query_engine.range(&query, None)); + dataframe_ui( + ctx, + ui, + query_engine.range(&query, None), + &mut state.expended_rows_cache, + ); } }; diff --git a/crates/viewer/re_ui/src/design_tokens.rs b/crates/viewer/re_ui/src/design_tokens.rs index 53c90d8449a6..37504e8df0fd 100644 --- a/crates/viewer/re_ui/src/design_tokens.rs +++ b/crates/viewer/re_ui/src/design_tokens.rs @@ -289,7 +289,7 @@ impl DesignTokens { } pub fn table_line_height() -> f32 { - 16.0 // should be big enough to contain buttons, i.e. egui_style.spacing.interact_size.y + 20.0 // should be big enough to contain buttons, i.e. egui_style.spacing.interact_size.y } pub fn table_header_height() -> f32 { diff --git a/tests/python/instance_count_test/instance_count_test.py b/tests/python/instance_count_test/instance_count_test.py new file mode 100644 index 000000000000..82ec02c2f838 --- /dev/null +++ b/tests/python/instance_count_test/instance_count_test.py @@ -0,0 +1,55 @@ +# Log data with various numbers of instances for test purposes, e.g. UI test, joining, etc. + +from __future__ import annotations + +import argparse +import random +import math + +import numpy as np + +import rerun as rr + + +def log_small_point_clouds(seed: int | None = None) -> None: + """Logs a time-varying point cloud with often partial color information.""" + if seed is not None: + random.seed(seed) + np.random.seed(seed) + + N = 1000 + + for i in range(N): + num_points = random.randint(1, 20) + num_colors = random.randint(1, num_points) + x = i / N * 2 + y = math.sin(i / N * 2 * math.pi) + spread = random.random() + 0.2 + + points_x = np.ones(num_points) * x + points_y = np.linspace(y - spread, y + spread, num_points) + + rr.log( + "small_point_cloud/pts", + rr.Points2D( + positions=np.vstack([points_x, points_y]).T, + radii=-4, + colors=np.random.randint(0, 255, size=(num_colors, 4)), + ), + ) + + rr.log("small_point_cloud/bbox", rr.Boxes2D(centers=[[1, 0]], sizes=[[3, 5]]), static=True) + + +def main() -> None: + parser = argparse.ArgumentParser(description="Log some data with varying number of instances for testing purposes") + rr.script_add_args(parser) + args = parser.parse_args() + + rr.script_setup(args, "rerun_example_instance_count_test") + + log_small_point_clouds() + + +if __name__ == "__main__": + main() From d80ef25f84bee6f6ca5a3970b3d07eef33fe2811 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 11 Sep 2024 17:26:37 +0200 Subject: [PATCH 02/19] Proper UI for collapsing/uncollapsing instances --- Cargo.lock | 2 +- .../viewer/re_space_view_dataframe/Cargo.toml | 2 +- .../src/dataframe_ui.rs | 110 +++++++++++++++--- .../src/display_record_batch.rs | 5 +- .../src/expanded_rows.rs | 16 ++- crates/viewer/re_ui/data/icons/collapse.png | Bin 0 -> 232 bytes crates/viewer/re_ui/data/icons/expand.png | Bin 0 -> 277 bytes crates/viewer/re_ui/src/icons.rs | 3 + 8 files changed, 113 insertions(+), 25 deletions(-) create mode 100644 crates/viewer/re_ui/data/icons/collapse.png create mode 100644 crates/viewer/re_ui/data/icons/expand.png diff --git a/Cargo.lock b/Cargo.lock index 8f499981fb6f..6b0fe8a44169 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1947,7 +1947,7 @@ dependencies = [ [[package]] name = "egui_table" version = "0.28.1" -source = "git+https://github.com/rerun-io/egui_table.git?rev=36887f386bdd5a324c0ae06f1cae5d73524c9b77#36887f386bdd5a324c0ae06f1cae5d73524c9b77" +source = "git+https://github.com/rerun-io/egui_table.git?rev=a3efe0bddfdb7c684cdaa478b5c0e979d238ea98#a3efe0bddfdb7c684cdaa478b5c0e979d238ea98" dependencies = [ "egui", "serde", diff --git a/crates/viewer/re_space_view_dataframe/Cargo.toml b/crates/viewer/re_space_view_dataframe/Cargo.toml index 537bfc7c96e1..6b2dc2a71be7 100644 --- a/crates/viewer/re_space_view_dataframe/Cargo.toml +++ b/crates/viewer/re_space_view_dataframe/Cargo.toml @@ -36,7 +36,7 @@ re_viewport_blueprint.workspace = true anyhow.workspace = true egui_extras.workspace = true -egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "36887f386bdd5a324c0ae06f1cae5d73524c9b77" } # PR #14 as of 2024-09-11 +egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "a3efe0bddfdb7c684cdaa478b5c0e979d238ea98" } # PR #14 as of 2024-09-11 egui.workspace = true itertools.workspace = true thiserror.workspace = true diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 0719875de9a8..6a36c006c7c0 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -258,6 +258,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { } }; + //TODO: this is getting wild, refactor in some functions let cell_ui = |ui: &mut egui::Ui| { if let Some(BatchRef { batch_idx, row_idx }) = display_data.batch_ref_from_row.get(&cell.row_nr).copied() @@ -311,26 +312,47 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { let mut sub_cell_ui = ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); - if instance_index == None && instance_count > 1 { - if sub_cell_ui - .button(format!("{instance_count} instances")) - .clicked() - { + if instance_index.is_none() && instance_count > 1 { + let cell_clicked = cell_with_hover_button_ui( + &mut sub_cell_ui, + Some(&re_ui::icons::EXPAND), + |ui| { + ui.label(format!("{instance_count} instances")); + }, + ); + + if cell_clicked { if instance_count == row_expansion { - self.expanded_rows.expend_row(cell.row_nr, 0); + self.expanded_rows.expand_row(cell.row_nr, 0); } else { - self.expanded_rows.expend_row(cell.row_nr, instance_count) + self.expanded_rows.expand_row(cell.row_nr, instance_count); } } } else { - column.data_ui( - self.ctx, + let has_collapse_button = if let Some(instance_index) = instance_index { + instance_index < instance_count + } else { + false + }; + + let cell_clicked = cell_with_hover_button_ui( &mut sub_cell_ui, - row_id, - &latest_at_query, - row_idx, - instance_index, + has_collapse_button.then_some(&re_ui::icons::COLLAPSE), + |ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + row_idx, + instance_index, + ); + }, ); + + if cell_clicked { + self.expanded_rows.expand_row(cell.row_nr, 0); + } } } } else { @@ -346,9 +368,8 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .show(ui, cell_ui); } - fn row_top_offset(&self, ctx: &egui::Context, table_id_salt: egui::Id, row_nr: u64) -> f32 { - self.expanded_rows - .row_top_offset(ctx, table_id_salt, row_nr) + fn row_top_offset(&self, ctx: &egui::Context, table_id: egui::Id, row_nr: u64) -> f32 { + self.expanded_rows.row_top_offset(ctx, table_id, row_nr) } fn default_row_height(&self) -> f32 { @@ -453,3 +474,60 @@ fn error_ui(ui: &mut egui::Ui, error: impl AsRef) { ui.error_label(error); re_log::warn_once!("{error}"); } + +/// Draw some cell content with an optional, right-aligned, on-hover button. +/// +/// If no icon is provided, no button is shown. Returns true if the button was shown and the cell +/// was clicked. +// TODO(ab, emilk): ideally, egui::Sides should work for that, but it doesn't yet support the +// symmetric behaviour (left variable width, right fixed width). +fn cell_with_hover_button_ui( + ui: &mut egui::Ui, + icon: Option<&'static re_ui::Icon>, + cell_content: impl FnOnce(&mut egui::Ui), +) -> bool { + let Some(icon) = icon else { + cell_content(ui); + return false; + }; + + let (is_hovering_cell, is_clicked) = ui.input(|i| { + ( + i.pointer + .interact_pos() + .is_some_and(|pos| ui.max_rect().contains(pos)), + i.pointer.button_clicked(egui::PointerButton::Primary), + ) + }); + + if is_hovering_cell { + let mut content_rect = ui.max_rect(); + content_rect.max.x = (content_rect.max.x + - re_ui::DesignTokens::small_icon_size().x + - re_ui::DesignTokens::text_to_icon_padding()) + .at_least(content_rect.min.x); + + let button_rect = egui::Rect::from_x_y_ranges( + (content_rect.max.x + re_ui::DesignTokens::text_to_icon_padding()) + ..=ui.max_rect().max.x, + ui.max_rect().y_range(), + ); + + let mut content_ui = ui.new_child(egui::UiBuilder::new().max_rect(content_rect)); + cell_content(&mut content_ui); + + let mut button_ui = ui.new_child(egui::UiBuilder::new().max_rect(button_rect)); + button_ui.visuals_mut().widgets.hovered.weak_bg_fill = egui::Color32::TRANSPARENT; + button_ui.visuals_mut().widgets.active.weak_bg_fill = egui::Color32::TRANSPARENT; + button_ui.add(egui::Button::image( + icon.as_image() + .fit_to_exact_size(re_ui::DesignTokens::small_icon_size()) + .tint(button_ui.visuals().widgets.hovered.text_color()), + )); + + is_clicked + } else { + cell_content(ui); + false + } +} diff --git a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs index 1a0c8cf04f0c..cb2d5e55a46f 100644 --- a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs +++ b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs @@ -123,7 +123,10 @@ impl ComponentData { ) { let data = match self { Self::Null => { - ui.label("null"); + // don't repeat the null value when expanding instances + if instance_index.is_none() { + ui.label("null"); + } return; } Self::ListArray(list_array) => list_array diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index 7e7c9baac321..633c3c673de3 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -75,24 +75,28 @@ impl<'a> ExpandedRows<'a> { pub(crate) fn row_top_offset( &self, ctx: &egui::Context, - id_salt: egui::Id, + table_id: egui::Id, row_nr: u64, ) -> f32 { self.cache .expanded_rows .range(0..row_nr) .map(|(expanded_row_nr, expanded)| { - let how_expanded = ctx.animate_bool(id_salt.with(expanded_row_nr), *expanded > 0); + let how_expanded = ctx.animate_bool(table_id.with(expanded_row_nr), *expanded > 0); how_expanded * *expanded as f32 * self.row_height }) .sum::() + row_nr as f32 * self.row_height } - pub(crate) fn expend_row(&mut self, row_nr: u64, additional_row_space: u64) { - self.cache - .expanded_rows - .insert(row_nr, additional_row_space); + pub(crate) fn expand_row(&mut self, row_nr: u64, additional_row_space: u64) { + if additional_row_space == 0 { + self.cache.expanded_rows.remove(&row_nr); + } else { + self.cache + .expanded_rows + .insert(row_nr, additional_row_space); + } } /// Return by how much row space this row is expended. diff --git a/crates/viewer/re_ui/data/icons/collapse.png b/crates/viewer/re_ui/data/icons/collapse.png new file mode 100644 index 0000000000000000000000000000000000000000..f0983e45834a647cc276691d9fd20407c439029d GIT binary patch literal 232 zcmeAS@N?(olHy`uVBq!ia0vp^G9b*s1|*Ak?@s|zoCO|{#S9E$svykh8Km+7D9BhG zmF{Fa=?fHkC4GIEn56>6e&uWrc7&>dg z#6_Gw&K^gYJFPz+-}}L_@rS~K-U;99_-iT+e(< a!?yIzS&u)Z?u&sAVeoYIb6Mw<&;$Th?p9*} literal 0 HcmV?d00001 diff --git a/crates/viewer/re_ui/data/icons/expand.png b/crates/viewer/re_ui/data/icons/expand.png new file mode 100644 index 0000000000000000000000000000000000000000..3d9ae99ae7c52669f94385b7d55fa133827fce4a GIT binary patch literal 277 zcmeAS@N?(olHy`uVBq!ia0vp^G9b*s1|*Ak?@s|zoCO|{#S9E$svykh8Km+7D9BhG zx0U3CNe#(uL*448*`D%JIH_SlcsAk z{?$AF&FhXj)8rf2W_V#i;fpiIZJ(#@Y88-EuAMNC>5%H1w&3DJ$6h`-cyMR5t(Xqn*o VPOJ9)b)c&mJYD@<);T3K0RYR_Y^wkO literal 0 HcmV?d00001 diff --git a/crates/viewer/re_ui/src/icons.rs b/crates/viewer/re_ui/src/icons.rs index 7a03285b816b..da9849aed9bb 100644 --- a/crates/viewer/re_ui/src/icons.rs +++ b/crates/viewer/re_ui/src/icons.rs @@ -57,6 +57,9 @@ pub const LEFT_PANEL_TOGGLE: Icon = icon_from_path!("../data/icons/left_panel_to pub const MINIMIZE: Icon = icon_from_path!("../data/icons/minimize.png"); pub const MAXIMIZE: Icon = icon_from_path!("../data/icons/maximize.png"); +pub const COLLAPSE: Icon = icon_from_path!("../data/icons/collapse.png"); +pub const EXPAND: Icon = icon_from_path!("../data/icons/expand.png"); + pub const VISIBLE: Icon = icon_from_path!("../data/icons/visible.png"); pub const INVISIBLE: Icon = icon_from_path!("../data/icons/invisible.png"); From b4b0d5cde083fafa07dd632d9295bdfc0ae9be02 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 11 Sep 2024 17:54:49 +0200 Subject: [PATCH 03/19] =?UTF-8?q?Proper=20handling=20of=20"xx=20more?= =?UTF-8?q?=E2=80=A6"=20instances?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/dataframe_ui.rs | 68 +++++++++++++------ .../src/expanded_rows.rs | 4 ++ 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 6a36c006c7c0..dd7eb92d3c6e 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -297,6 +297,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .take(row_expansion as usize + 1); for (sub_cell_index, instance_index) in instance_indices.enumerate() { + // TODO: have an helper to split UIs that way let sub_cell_rect = egui::Rect::from_min_size( ui.cursor().min + egui::vec2( @@ -323,35 +324,62 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { if cell_clicked { if instance_count == row_expansion { - self.expanded_rows.expand_row(cell.row_nr, 0); + self.expanded_rows.collapse_row(cell.row_nr); } else { self.expanded_rows.expand_row(cell.row_nr, instance_count); } } } else { - let has_collapse_button = if let Some(instance_index) = instance_index { - instance_index < instance_count + let has_collapse_button = instance_index + .is_some_and(|instance_index| instance_index < instance_count); + + let remaining_instances = if sub_cell_index as u64 == row_expansion { + instance_index.and_then(|instance_index| { + let remaining = instance_count + .saturating_sub(instance_index) + .saturating_sub(1); + if remaining > 0 { + // +1 is because the "X more…" line takes one instance spot + Some(remaining + 1) + } else { + None + } + }) } else { - false + None }; - let cell_clicked = cell_with_hover_button_ui( - &mut sub_cell_ui, - has_collapse_button.then_some(&re_ui::icons::COLLAPSE), - |ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - row_idx, - instance_index, - ); - }, - ); + if let Some(remaining_instances) = remaining_instances { + let cell_clicked = cell_with_hover_button_ui( + &mut sub_cell_ui, + Some(&re_ui::icons::EXPAND), + |ui| { + ui.label(format!("{remaining_instances} more…")); + }, + ); - if cell_clicked { - self.expanded_rows.expand_row(cell.row_nr, 0); + if cell_clicked { + self.expanded_rows.expand_row(cell.row_nr, instance_count); + } + } else { + let cell_clicked = cell_with_hover_button_ui( + &mut sub_cell_ui, + has_collapse_button.then_some(&re_ui::icons::COLLAPSE), + |ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + row_idx, + instance_index, + ); + }, + ); + + if cell_clicked { + self.expanded_rows.collapse_row(cell.row_nr); + } } } } diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index 633c3c673de3..2df8419c129c 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -89,6 +89,10 @@ impl<'a> ExpandedRows<'a> { + row_nr as f32 * self.row_height } + pub(crate) fn collapse_row(&mut self, row_nr: u64) { + self.expand_row(row_nr, 0); + } + pub(crate) fn expand_row(&mut self, row_nr: u64, additional_row_space: u64) { if additional_row_space == 0 { self.cache.expanded_rows.remove(&row_nr); From 1fa7fb5f00a6ac13a7f3e18c1d3720d6fdc6c5ea Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 10:53:53 +0200 Subject: [PATCH 04/19] Fix animation + some code cleaning --- Cargo.lock | 2 +- .../viewer/re_space_view_dataframe/Cargo.toml | 2 +- .../src/dataframe_ui.rs | 23 +++-- .../src/display_record_batch.rs | 4 + .../src/expanded_rows.rs | 84 ++++++++++++++----- 5 files changed, 84 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6718b116dae6..35320fea0898 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1947,7 +1947,7 @@ dependencies = [ [[package]] name = "egui_table" version = "0.28.1" -source = "git+https://github.com/rerun-io/egui_table.git?rev=a3efe0bddfdb7c684cdaa478b5c0e979d238ea98#a3efe0bddfdb7c684cdaa478b5c0e979d238ea98" +source = "git+https://github.com/rerun-io/egui_table.git?rev=1c5db2cc3c4c8db79e85824f7ca6cca87110111f#1c5db2cc3c4c8db79e85824f7ca6cca87110111f" dependencies = [ "egui", "serde", diff --git a/crates/viewer/re_space_view_dataframe/Cargo.toml b/crates/viewer/re_space_view_dataframe/Cargo.toml index 6b2dc2a71be7..80e23ad49ef5 100644 --- a/crates/viewer/re_space_view_dataframe/Cargo.toml +++ b/crates/viewer/re_space_view_dataframe/Cargo.toml @@ -36,7 +36,7 @@ re_viewport_blueprint.workspace = true anyhow.workspace = true egui_extras.workspace = true -egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "a3efe0bddfdb7c684cdaa478b5c0e979d238ea98" } # PR #14 as of 2024-09-11 +egui_table = { git = "https://github.com/rerun-io/egui_table.git", rev = "1c5db2cc3c4c8db79e85824f7ca6cca87110111f" } # main as of 2024-09-12 egui.workspace = true itertools.workspace = true thiserror.workspace = true diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index dd7eb92d3c6e..e36b388e5980 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -310,6 +310,11 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { ), ); + // dont draw unnecessary + if !ui.max_rect().intersects(sub_cell_rect) { + return; + } + let mut sub_cell_ui = ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); @@ -326,7 +331,8 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { if instance_count == row_expansion { self.expanded_rows.collapse_row(cell.row_nr); } else { - self.expanded_rows.expand_row(cell.row_nr, instance_count); + self.expanded_rows + .set_row_expansion(cell.row_nr, instance_count); } } } else { @@ -359,7 +365,8 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { ); if cell_clicked { - self.expanded_rows.expand_row(cell.row_nr, instance_count); + self.expanded_rows + .set_row_expansion(cell.row_nr, instance_count); } } else { let cell_clicked = cell_with_hover_button_ui( @@ -396,8 +403,8 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .show(ui, cell_ui); } - fn row_top_offset(&self, ctx: &egui::Context, table_id: egui::Id, row_nr: u64) -> f32 { - self.expanded_rows.row_top_offset(ctx, table_id, row_nr) + fn row_top_offset(&self, _ctx: &egui::Context, _table_id: egui::Id, row_nr: u64) -> f32 { + self.expanded_rows.row_top_offset(row_nr) } fn default_row_height(&self) -> f32 { @@ -414,8 +421,8 @@ fn dataframe_ui_impl( ) { re_tracing::profile_function!(); - //TODO: actually make that unique! - let id = ui.id().with("__dataframe__"); + //TODO: actually make that unique!!! + let id_salt = egui::Id::new("__dataframe__"); let schema = query_handle.schema(); let (header_groups, header_entity_paths) = column_groups_for_entity(schema); @@ -432,6 +439,8 @@ fn dataframe_ui_impl( "No row data, `fetch_columns_and_rows` not called." )), expanded_rows: ExpandedRows::new( + ui.ctx().clone(), + ui.make_persistent_id(id_salt).with("expanded_rows"), expanded_rows_cache, query_handle.query_expression(), re_ui::DesignTokens::table_line_height(), @@ -445,7 +454,7 @@ fn dataframe_ui_impl( egui::Frame::none().inner_margin(5.0).show(ui, |ui| { egui_table::Table::new() - .id_salt(id) + .id_salt(id_salt) .columns( schema .iter() diff --git a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs index cb2d5e55a46f..86bf8e149619 100644 --- a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs +++ b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs @@ -31,6 +31,9 @@ pub(crate) enum DisplayRecordBatchError { UnexpectedComponentColumnDataType(String, ArrowDataType), } +/// A single column of component data. +/// +/// Abstracts over the different possible arrow representation of component data. pub(crate) enum ComponentData { Null, ListArray(ArrowListArray), @@ -163,6 +166,7 @@ impl ComponentData { } } +/// A single column of data in a record batch. pub(crate) enum DisplayColumn { RowId { row_id_times: ArrowPrimitiveArray, diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index 2df8419c129c..fb0e5f4e6526 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -51,60 +51,100 @@ impl ExpandedRowsCache { /// /// This is a short-lived struct to be created every frame. The persistent state is stored in /// [`ExpandedRowsCache`]. +/// +/// Uses egui's animation support to animate the row expansion/contraction. For this to work: +/// - When collapsed, the row entry must be set to 0 instead of being removed. Otherwise, it will no +/// longer be "seen" by the animation code. Technically, it could be removed _after_ the +/// animation completes, but it's not worth the complexity. +/// - When the row is first expanded, for the animation to work, it must be immediately seeded to 0 +/// for the animation to have a starting point. pub(crate) struct ExpandedRows<'a> { /// Base row height. row_height: f32, /// Cache containing the row expanded-ness. cache: &'a mut ExpandedRowsCache, + + /// [`egui::Context`] used to animate the row expansion. + egui_ctx: egui::Context, + + /// [`egui::Id`] used to store the animation state. + id: egui::Id, } impl<'a> ExpandedRows<'a> { + /// Create a new [`ExpandedRows`] instance. + /// + /// `egui_ctx` is used to animate the row expansion + /// `id` is used to store the animation state, make it persistent and unique + /// `query_expression` is used to invalidate the cache pub(crate) fn new( + egui_ctx: egui::Context, + id: egui::Id, cache: &'a mut ExpandedRowsCache, query_expression: impl Into, row_height: f32, ) -> Self { - // validate the cache + // (in-)validate the cache cache.set_query(query_expression.into()); - Self { row_height, cache } + Self { + row_height, + cache, + egui_ctx, + id, + } } /// Implementation for [`egui_table::Table::row_top_offset`]. - pub(crate) fn row_top_offset( - &self, - ctx: &egui::Context, - table_id: egui::Id, - row_nr: u64, - ) -> f32 { + pub(crate) fn row_top_offset(&self, row_nr: u64) -> f32 { self.cache .expanded_rows .range(0..row_nr) .map(|(expanded_row_nr, expanded)| { - let how_expanded = ctx.animate_bool(table_id.with(expanded_row_nr), *expanded > 0); - how_expanded * *expanded as f32 * self.row_height + self.egui_ctx.animate_value_with_time( + self.row_id(*expanded_row_nr), + *expanded as f32 * self.row_height, + self.egui_ctx.style().animation_time, + ) }) .sum::() + row_nr as f32 * self.row_height } - pub(crate) fn collapse_row(&mut self, row_nr: u64) { - self.expand_row(row_nr, 0); + /// Return by how much row space this row is expended. + pub(crate) fn row_expansion(&self, row_nr: u64) -> u64 { + self.cache.expanded_rows.get(&row_nr).copied().unwrap_or(0) } - pub(crate) fn expand_row(&mut self, row_nr: u64, additional_row_space: u64) { - if additional_row_space == 0 { - self.cache.expanded_rows.remove(&row_nr); - } else { - self.cache - .expanded_rows - .insert(row_nr, additional_row_space); + /// Set the expansion of a row. + /// + /// Units are in extra row heights. + pub(crate) fn set_row_expansion(&mut self, row_nr: u64, additional_row_space: u64) { + // Note: don't delete the entry when set to 0, this breaks animation. + + // If this is the first time this row is expended, we must seed the corresponding animation + // cache. + if !self.cache.expanded_rows.contains_key(&row_nr) { + self.egui_ctx.animate_value_with_time( + self.row_id(row_nr), + 0.0, + self.egui_ctx.style().animation_time, + ); } + + self.cache + .expanded_rows + .insert(row_nr, additional_row_space); } - /// Return by how much row space this row is expended. - pub(crate) fn row_expansion(&self, row_nr: u64) -> u64 { - self.cache.expanded_rows.get(&row_nr).copied().unwrap_or(0) + /// Collapse a row. + pub(crate) fn collapse_row(&mut self, row_nr: u64) { + self.set_row_expansion(row_nr, 0); + } + + #[inline] + fn row_id(&self, row_nr: u64) -> egui::Id { + self.id.with(row_nr) } } From 69f394abb4eca8a67629ba49c1a06a52bcfdbee7 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 12:22:33 +0200 Subject: [PATCH 05/19] More cleaning and lint fix --- .../src/dataframe_ui.rs | 221 ++++++++++-------- .../src/expanded_rows.rs | 4 +- .../instance_count_test.py | 4 +- 3 files changed, 134 insertions(+), 95 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index e36b388e5980..8b66f4a260c9 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -250,6 +250,11 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { debug_assert!(cell.row_nr < self.num_rows, "Bug in egui_table"); + // best effort to get a proper column autosizing behaviour + if ui.is_sizing_pass() { + ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); + } + let display_data = match &self.display_data { Ok(display_data) => display_data, Err(err) => { @@ -258,7 +263,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { } }; - //TODO: this is getting wild, refactor in some functions + // TODO(ab): this is getting wild and should be refactored let cell_ui = |ui: &mut egui::Ui| { if let Some(BatchRef { batch_idx, row_idx }) = display_data.batch_ref_from_row.get(&cell.row_nr).copied() @@ -296,100 +301,85 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .chain((0..instance_count).map(Option::Some)) .take(row_expansion as usize + 1); - for (sub_cell_index, instance_index) in instance_indices.enumerate() { - // TODO: have an helper to split UIs that way - let sub_cell_rect = egui::Rect::from_min_size( - ui.cursor().min - + egui::vec2( - 0.0, - sub_cell_index as f32 * re_ui::DesignTokens::table_line_height(), - ), - egui::vec2( - ui.available_width(), - re_ui::DesignTokens::table_line_height(), - ), - ); - - // dont draw unnecessary - if !ui.max_rect().intersects(sub_cell_rect) { - return; - } - - let mut sub_cell_ui = - ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); - - if instance_index.is_none() && instance_count > 1 { - let cell_clicked = cell_with_hover_button_ui( - &mut sub_cell_ui, - Some(&re_ui::icons::EXPAND), - |ui| { - ui.label(format!("{instance_count} instances")); - }, - ); + let sub_cell_content = + |ui: &mut egui::Ui, + expanded_rows: &mut ExpandedRows<'_>, + sub_cell_index: usize, + instance_index: Option| { + if instance_index.is_none() && instance_count > 1 { + let cell_clicked = + cell_with_hover_button_ui(ui, Some(&re_ui::icons::EXPAND), |ui| { + ui.label(format!("{instance_count} instances")); + }); - if cell_clicked { - if instance_count == row_expansion { - self.expanded_rows.collapse_row(cell.row_nr); - } else { - self.expanded_rows - .set_row_expansion(cell.row_nr, instance_count); - } - } - } else { - let has_collapse_button = instance_index - .is_some_and(|instance_index| instance_index < instance_count); - - let remaining_instances = if sub_cell_index as u64 == row_expansion { - instance_index.and_then(|instance_index| { - let remaining = instance_count - .saturating_sub(instance_index) - .saturating_sub(1); - if remaining > 0 { - // +1 is because the "X more…" line takes one instance spot - Some(remaining + 1) + if cell_clicked { + if instance_count == row_expansion { + expanded_rows.collapse_row(cell.row_nr); } else { - None + expanded_rows.set_row_expansion(cell.row_nr, instance_count); } - }) - } else { - None - }; - - if let Some(remaining_instances) = remaining_instances { - let cell_clicked = cell_with_hover_button_ui( - &mut sub_cell_ui, - Some(&re_ui::icons::EXPAND), - |ui| { - ui.label(format!("{remaining_instances} more…")); - }, - ); - - if cell_clicked { - self.expanded_rows - .set_row_expansion(cell.row_nr, instance_count); } } else { - let cell_clicked = cell_with_hover_button_ui( - &mut sub_cell_ui, - has_collapse_button.then_some(&re_ui::icons::COLLAPSE), - |ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - row_idx, - instance_index, - ); - }, - ); - - if cell_clicked { - self.expanded_rows.collapse_row(cell.row_nr); + let has_collapse_button = instance_index + .is_some_and(|instance_index| instance_index < instance_count); + + let remaining_instances = if sub_cell_index as u64 == row_expansion { + instance_index.and_then(|instance_index| { + let remaining = instance_count + .saturating_sub(instance_index) + .saturating_sub(1); + if remaining > 0 { + // +1 is because the "X more…" line takes one instance spot + Some(remaining + 1) + } else { + None + } + }) + } else { + None + }; + + if let Some(remaining_instances) = remaining_instances { + let cell_clicked = cell_with_hover_button_ui( + ui, + Some(&re_ui::icons::EXPAND), + |ui| { + ui.label(format!("{remaining_instances} more…")); + }, + ); + + if cell_clicked { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } else { + let cell_clicked = cell_with_hover_button_ui( + ui, + has_collapse_button.then_some(&re_ui::icons::COLLAPSE), + |ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + row_idx, + instance_index, + ); + }, + ); + + if cell_clicked { + expanded_rows.collapse_row(cell.row_nr); + } } } - } - } + }; + + sub_cell_ui( + ui, + &mut self.expanded_rows, + instance_indices, + sub_cell_content, + ); } else { error_ui( ui, @@ -421,8 +411,9 @@ fn dataframe_ui_impl( ) { re_tracing::profile_function!(); - //TODO: actually make that unique!!! - let id_salt = egui::Id::new("__dataframe__"); + // Salt everything against the query expression, as different queries may lead to entirely + // different tables, etc. + let id_salt = egui::Id::new("__dataframe__").with(query_handle.query_expression()); let schema = query_handle.schema(); let (header_groups, header_entity_paths) = column_groups_for_entity(schema); @@ -517,7 +508,7 @@ fn error_ui(ui: &mut egui::Ui, error: impl AsRef) { /// If no icon is provided, no button is shown. Returns true if the button was shown and the cell /// was clicked. // TODO(ab, emilk): ideally, egui::Sides should work for that, but it doesn't yet support the -// symmetric behaviour (left variable width, right fixed width). +// symmetric behavior (left variable width, right fixed width). fn cell_with_hover_button_ui( ui: &mut egui::Ui, icon: Option<&'static re_ui::Icon>, @@ -528,6 +519,12 @@ fn cell_with_hover_button_ui( return false; }; + if ui.is_sizing_pass() { + // we don't need space for the icon since it only shows on hover + cell_content(ui); + return false; + } + let (is_hovering_cell, is_clicked) = ui.input(|i| { ( i.pointer @@ -568,3 +565,45 @@ fn cell_with_hover_button_ui( false } } + +/// Helper to draw individual rows (aka sub-cell) into an expanded cell in a table. +/// +/// `context`: whatever mutable context is necessary for the `sub_cell_content` +/// `sub_cell_data`: the data to be displayed in each sub-cell +/// `sub_cell_content`: the function to draw the content of each sub-cell +fn sub_cell_ui( + ui: &mut egui::Ui, + context: &mut Ctx, + sub_cell_data: impl Iterator, + sub_cell_content: impl Fn(&mut egui::Ui, &mut Ctx, usize, I), +) { + for (sub_cell_index, item_data) in sub_cell_data.enumerate() { + let sub_cell_rect = egui::Rect::from_min_size( + ui.cursor().min + + egui::vec2( + 0.0, + sub_cell_index as f32 * re_ui::DesignTokens::table_line_height(), + ), + egui::vec2( + ui.available_width(), + re_ui::DesignTokens::table_line_height(), + ), + ); + + // During animation, there may be more sub-cells than can possibly fit. If so, no point in + // continuing to draw them. + if !ui.max_rect().intersects(sub_cell_rect) { + return; + } + + if !ui.is_rect_visible(sub_cell_rect) { + continue; + // TODO(ab): detect that we are below any possible screen, in which case we can stop + // entirely. + } + + let mut sub_cell_ui = ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); + + sub_cell_content(&mut sub_cell_ui, context, sub_cell_index, item_data); + } +} diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index fb0e5f4e6526..0a5794b9ffd9 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use re_chunk_store::{LatestAtQueryExpression, RangeQueryExpression}; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub(crate) enum QueryExpression { LatestAt(re_chunk_store::LatestAtQueryExpression), Range(re_chunk_store::RangeQueryExpression), @@ -96,7 +96,7 @@ impl<'a> ExpandedRows<'a> { } } - /// Implementation for [`egui_table::Table::row_top_offset`]. + /// Implementation for [`egui_table::TableDelegate::row_top_offset`]. pub(crate) fn row_top_offset(&self, row_nr: u64) -> f32 { self.cache .expanded_rows diff --git a/tests/python/instance_count_test/instance_count_test.py b/tests/python/instance_count_test/instance_count_test.py index 82ec02c2f838..7e84ded3c9da 100644 --- a/tests/python/instance_count_test/instance_count_test.py +++ b/tests/python/instance_count_test/instance_count_test.py @@ -1,13 +1,13 @@ # Log data with various numbers of instances for test purposes, e.g. UI test, joining, etc. +# TODO(ab): move this to chunk zoo from __future__ import annotations import argparse -import random import math +import random import numpy as np - import rerun as rr From ec8efd4a95b681aaa15adabbde2f3e293b917137 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 12:25:55 +0200 Subject: [PATCH 06/19] Make the collapse/expand button the same color as inactive text --- crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 8b66f4a260c9..b99487a3cdb0 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -556,7 +556,7 @@ fn cell_with_hover_button_ui( button_ui.add(egui::Button::image( icon.as_image() .fit_to_exact_size(re_ui::DesignTokens::small_icon_size()) - .tint(button_ui.visuals().widgets.hovered.text_color()), + .tint(button_ui.visuals().widgets.noninteractive.text_color()), )); is_clicked From 6d4602528a6bdd40060bc5a8617a3af7206c092c Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 13:31:16 +0200 Subject: [PATCH 07/19] US spelling --- crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs | 2 +- crates/viewer/re_ui/src/list_item/list_item.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index b99487a3cdb0..f8a866410a3f 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -250,7 +250,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { debug_assert!(cell.row_nr < self.num_rows, "Bug in egui_table"); - // best effort to get a proper column autosizing behaviour + // best effort to get a proper column auto-sizing behavior if ui.is_sizing_pass() { ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); } diff --git a/crates/viewer/re_ui/src/list_item/list_item.rs b/crates/viewer/re_ui/src/list_item/list_item.rs index f004bede9a7e..a6805054a46e 100644 --- a/crates/viewer/re_ui/src/list_item/list_item.rs +++ b/crates/viewer/re_ui/src/list_item/list_item.rs @@ -103,7 +103,7 @@ impl ListItem { /// Highlight the item as the current drop target. /// /// Use this while dragging, to highlight which container will receive the drop at any given time. - /// **Note**: this flag has otherwise no behavioural effect. It's up to the caller to set it when the item is + /// **Note**: this flag has otherwise no behavioral effect. It's up to the caller to set it when the item is /// being hovered (or otherwise selected as drop target) while a drag is in progress. #[inline] pub fn drop_target_style(mut self, drag_target: bool) -> Self { From f63728fac5c49b4b33c21dabc9f2b55caaa0edc8 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 15:06:06 +0200 Subject: [PATCH 08/19] Rewrite of the sub-cell logic, reads like a child book now [skip ci] --- .../src/dataframe_ui.rs | 144 +++++++++++------- 1 file changed, 92 insertions(+), 52 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index f8a866410a3f..33863b35f5b3 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -306,71 +306,117 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { expanded_rows: &mut ExpandedRows<'_>, sub_cell_index: usize, instance_index: Option| { - if instance_index.is_none() && instance_count > 1 { - let cell_clicked = - cell_with_hover_button_ui(ui, Some(&re_ui::icons::EXPAND), |ui| { - ui.label(format!("{instance_count} instances")); - }); - - if cell_clicked { - if instance_count == row_expansion { - expanded_rows.collapse_row(cell.row_nr); + /// What kinds of sub-cells might we encounter here? + enum SubcellKind { + /// Summary line with content that as zero or one instances, so cannot expand. + Summary, + + /// Summary line with >1 instances, so can be expanded. + SummaryWithExpand, + + /// A particular instance with the corresponding index. + Instance(u64 /* instance index */), + + /// There are more instances than available sub-cells, so this is a summary of how many there are left. + MoreInstancesSummary(u64 /* remaining instances */), + + /// Not enough instances to fill this sub-cell. + Blank, + } + + // The truth table that determines what kind of sub-cell we are dealing with. + let subcell_kind = match instance_index { + // First row with >1 instances. + None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, + + // First row with 0 or 1 instances. + None => SubcellKind::Summary, + + // Last sub-cell and possibly too many instances to display. + Some(instance_index) + if { + sub_cell_index as u64 == row_expansion + && instance_index < instance_count + } => + { + let remaining = instance_count + .saturating_sub(instance_index) + .saturating_sub(1); + if remaining > 0 { + // +1 is because the "X more…" line takes one instance spot + SubcellKind::MoreInstancesSummary(remaining + 1) } else { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); + SubcellKind::Instance(instance_index) } } - } else { - let has_collapse_button = instance_index - .is_some_and(|instance_index| instance_index < instance_count); - - let remaining_instances = if sub_cell_index as u64 == row_expansion { - instance_index.and_then(|instance_index| { - let remaining = instance_count - .saturating_sub(instance_index) - .saturating_sub(1); - if remaining > 0 { - // +1 is because the "X more…" line takes one instance spot - Some(remaining + 1) - } else { - None - } - }) - } else { - None - }; - if let Some(remaining_instances) = remaining_instances { - let cell_clicked = cell_with_hover_button_ui( + // Some sub-cell for which an instance exists. + Some(instance_index) if { instance_index < instance_count } => { + SubcellKind::Instance(instance_index) + } + + // Some sub-cell for which no instance exists. + Some(_) => SubcellKind::Blank, + }; + + match subcell_kind { + SubcellKind::Summary => { + column.data_ui( + self.ctx, ui, - Some(&re_ui::icons::EXPAND), - |ui| { - ui.label(format!("{remaining_instances} more…")); - }, + row_id, + &latest_at_query, + row_idx, + instance_index, ); + } + + SubcellKind::SummaryWithExpand => { + let cell_clicked = + cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{instance_count} instances")); + }); if cell_clicked { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); + if instance_count == row_expansion { + expanded_rows.collapse_row(cell.row_nr); + } else { + expanded_rows + .set_row_expansion(cell.row_nr, instance_count); + } } - } else { - let cell_clicked = cell_with_hover_button_ui( - ui, - has_collapse_button.then_some(&re_ui::icons::COLLAPSE), - |ui| { + } + + SubcellKind::Instance(instance_index) => { + let cell_clicked = + cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { column.data_ui( self.ctx, ui, row_id, &latest_at_query, row_idx, - instance_index, + Some(instance_index), ); - }, - ); + }); if cell_clicked { expanded_rows.collapse_row(cell.row_nr); } } + + SubcellKind::MoreInstancesSummary(remaining_instances) => { + let cell_clicked = + cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{remaining_instances} more…")); + }); + + if cell_clicked { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } + + SubcellKind::Blank => { /* nothing to show */ } } }; @@ -505,20 +551,14 @@ fn error_ui(ui: &mut egui::Ui, error: impl AsRef) { /// Draw some cell content with an optional, right-aligned, on-hover button. /// -/// If no icon is provided, no button is shown. Returns true if the button was shown and the cell -/// was clicked. +/// Returns true if the button was clicked. // TODO(ab, emilk): ideally, egui::Sides should work for that, but it doesn't yet support the // symmetric behavior (left variable width, right fixed width). fn cell_with_hover_button_ui( ui: &mut egui::Ui, - icon: Option<&'static re_ui::Icon>, + icon: &'static re_ui::Icon, cell_content: impl FnOnce(&mut egui::Ui), ) -> bool { - let Some(icon) = icon else { - cell_content(ui); - return false; - }; - if ui.is_sizing_pass() { // we don't need space for the icon since it only shows on hover cell_content(ui); From 4b134692592e19c371ea682da6b7f608919ecc72 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 15:15:13 +0200 Subject: [PATCH 09/19] Extract part of cell_ui into cell_ui_impl --- .../src/dataframe_ui.rs | 342 +++++++++--------- 1 file changed, 172 insertions(+), 170 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 33863b35f5b3..69c40d3b9bf1 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use std::ops::Range; use anyhow::Context; -use egui::NumExt as _; +use egui::{NumExt as _, Ui}; use itertools::Itertools; use re_chunk_store::{ColumnDescriptor, LatestAtQuery, RowId}; @@ -255,6 +255,24 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); } + egui::Frame::none() + .inner_margin(egui::Margin::symmetric(Self::LEFT_RIGHT_MARGIN, 0.0)) + .show(ui, |ui| { + self.cell_ui_impl(ui, cell); + }); + } + + fn row_top_offset(&self, _ctx: &egui::Context, _table_id: egui::Id, row_nr: u64) -> f32 { + self.expanded_rows.row_top_offset(row_nr) + } + + fn default_row_height(&self) -> f32 { + re_ui::DesignTokens::table_line_height() + } +} + +impl DataframeTableDelegate<'_> { + fn cell_ui_impl(&mut self, ui: &mut Ui, cell: &egui_table::CellInfo) { let display_data = match &self.display_data { Ok(display_data) => display_data, Err(err) => { @@ -263,188 +281,172 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { } }; - // TODO(ab): this is getting wild and should be refactored - let cell_ui = |ui: &mut egui::Ui| { - if let Some(BatchRef { batch_idx, row_idx }) = - display_data.batch_ref_from_row.get(&cell.row_nr).copied() - { - let batch = &display_data.display_record_batches[batch_idx]; - let column = &batch.columns()[cell.col_nr]; - - // compute the latest-at query for this row (used to display tooltips) - let timestamp = display_data - .query_time_column_index - .and_then(|col_idx| { - display_data.display_record_batches[batch_idx].columns()[col_idx] - .try_decode_time(row_idx) - }) - .unwrap_or(TimeInt::MAX); - let latest_at_query = LatestAtQuery::new(self.query_handle.timeline(), timestamp); - let row_id = display_data - .row_id_column_index - .and_then(|col_idx| { - display_data.display_record_batches[batch_idx].columns()[col_idx] - .try_decode_row_id(row_idx) - }) - .unwrap_or(RowId::ZERO); + let Some(BatchRef { + batch_idx, + row_idx: batch_row_idx, + }) = display_data.batch_ref_from_row.get(&cell.row_nr).copied() + else { + error_ui( + ui, + "Bug in egui_table: we didn't prefetch what was rendered!", + ); - if ui.is_sizing_pass() { - ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); - } else { - ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Truncate); - } + return; + }; - let instance_count = column.instance_count(row_idx); - let row_expansion = self.expanded_rows.row_expansion(cell.row_nr); + let batch = &display_data.display_record_batches[batch_idx]; + let column = &batch.columns()[cell.col_nr]; - let instance_indices = std::iter::once(None) - .chain((0..instance_count).map(Option::Some)) - .take(row_expansion as usize + 1); + // compute the latest-at query for this row (used to display tooltips) + let timestamp = display_data + .query_time_column_index + .and_then(|col_idx| { + display_data.display_record_batches[batch_idx].columns()[col_idx] + .try_decode_time(batch_row_idx) + }) + .unwrap_or(TimeInt::MAX); + let latest_at_query = LatestAtQuery::new(self.query_handle.timeline(), timestamp); + let row_id = display_data + .row_id_column_index + .and_then(|col_idx| { + display_data.display_record_batches[batch_idx].columns()[col_idx] + .try_decode_row_id(batch_row_idx) + }) + .unwrap_or(RowId::ZERO); - let sub_cell_content = - |ui: &mut egui::Ui, - expanded_rows: &mut ExpandedRows<'_>, - sub_cell_index: usize, - instance_index: Option| { - /// What kinds of sub-cells might we encounter here? - enum SubcellKind { - /// Summary line with content that as zero or one instances, so cannot expand. - Summary, + if ui.is_sizing_pass() { + ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); + } else { + ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Truncate); + } - /// Summary line with >1 instances, so can be expanded. - SummaryWithExpand, + let instance_count = column.instance_count(batch_row_idx); + let row_expansion = self.expanded_rows.row_expansion(cell.row_nr); - /// A particular instance with the corresponding index. - Instance(u64 /* instance index */), + let instance_indices = std::iter::once(None) + .chain((0..instance_count).map(Option::Some)) + .take(row_expansion as usize + 1); - /// There are more instances than available sub-cells, so this is a summary of how many there are left. - MoreInstancesSummary(u64 /* remaining instances */), + let sub_cell_content = |ui: &mut egui::Ui, + expanded_rows: &mut ExpandedRows<'_>, + sub_cell_index: usize, + instance_index: Option| { + /// What kinds of sub-cells might we encounter here? + enum SubcellKind { + /// Summary line with content that as zero or one instances, so cannot be expanded. + Summary, - /// Not enough instances to fill this sub-cell. - Blank, - } + /// Summary line with >1 instances, so can be expanded. + SummaryWithExpand, - // The truth table that determines what kind of sub-cell we are dealing with. - let subcell_kind = match instance_index { - // First row with >1 instances. - None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, - - // First row with 0 or 1 instances. - None => SubcellKind::Summary, - - // Last sub-cell and possibly too many instances to display. - Some(instance_index) - if { - sub_cell_index as u64 == row_expansion - && instance_index < instance_count - } => - { - let remaining = instance_count - .saturating_sub(instance_index) - .saturating_sub(1); - if remaining > 0 { - // +1 is because the "X more…" line takes one instance spot - SubcellKind::MoreInstancesSummary(remaining + 1) - } else { - SubcellKind::Instance(instance_index) - } - } - - // Some sub-cell for which an instance exists. - Some(instance_index) if { instance_index < instance_count } => { - SubcellKind::Instance(instance_index) - } - - // Some sub-cell for which no instance exists. - Some(_) => SubcellKind::Blank, - }; - - match subcell_kind { - SubcellKind::Summary => { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - row_idx, - instance_index, - ); - } - - SubcellKind::SummaryWithExpand => { - let cell_clicked = - cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{instance_count} instances")); - }); - - if cell_clicked { - if instance_count == row_expansion { - expanded_rows.collapse_row(cell.row_nr); - } else { - expanded_rows - .set_row_expansion(cell.row_nr, instance_count); - } - } - } - - SubcellKind::Instance(instance_index) => { - let cell_clicked = - cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - row_idx, - Some(instance_index), - ); - }); - - if cell_clicked { - expanded_rows.collapse_row(cell.row_nr); - } - } - - SubcellKind::MoreInstancesSummary(remaining_instances) => { - let cell_clicked = - cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{remaining_instances} more…")); - }); - - if cell_clicked { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); - } - } - - SubcellKind::Blank => { /* nothing to show */ } - } - }; - - sub_cell_ui( - ui, - &mut self.expanded_rows, - instance_indices, - sub_cell_content, - ); - } else { - error_ui( - ui, - "Bug in egui_table: we didn't prefetch what was rendered!", - ); + /// A particular instance with the corresponding index. + Instance(u64 /* instance index */), + + /// There are more instances than available sub-cells, so this is a summary of how many there are left. + MoreInstancesSummary(u64 /* remaining instances */), + + /// Not enough instances to fill this sub-cell. + Blank, } - }; - egui::Frame::none() - .inner_margin(egui::Margin::symmetric(Self::LEFT_RIGHT_MARGIN, 0.0)) - .show(ui, cell_ui); - } + // The truth table that determines what kind of sub-cell we are dealing with. + let subcell_kind = match instance_index { + // First row with >1 instances. + None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, + + // First row with 0 or 1 instances. + None => SubcellKind::Summary, + + // Last sub-cell and possibly too many instances to display. + Some(instance_index) + if { + sub_cell_index as u64 == row_expansion && instance_index < instance_count + } => + { + let remaining = instance_count + .saturating_sub(instance_index) + .saturating_sub(1); + if remaining > 0 { + // +1 is because the "X more…" line takes one instance spot + SubcellKind::MoreInstancesSummary(remaining + 1) + } else { + SubcellKind::Instance(instance_index) + } + } - fn row_top_offset(&self, _ctx: &egui::Context, _table_id: egui::Id, row_nr: u64) -> f32 { - self.expanded_rows.row_top_offset(row_nr) - } + // Some sub-cell for which an instance exists. + Some(instance_index) if { instance_index < instance_count } => { + SubcellKind::Instance(instance_index) + } - fn default_row_height(&self) -> f32 { - re_ui::DesignTokens::table_line_height() + // Some sub-cell for which no instance exists. + Some(_) => SubcellKind::Blank, + }; + + match subcell_kind { + SubcellKind::Summary => { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + batch_row_idx, + instance_index, + ); + } + + SubcellKind::SummaryWithExpand => { + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{instance_count} instances")); + }); + + if cell_clicked { + if instance_count == row_expansion { + expanded_rows.collapse_row(cell.row_nr); + } else { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } + } + + SubcellKind::Instance(instance_index) => { + let cell_clicked = + cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + batch_row_idx, + Some(instance_index), + ); + }); + + if cell_clicked { + expanded_rows.collapse_row(cell.row_nr); + } + } + + SubcellKind::MoreInstancesSummary(remaining_instances) => { + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{remaining_instances} more…")); + }); + + if cell_clicked { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } + + SubcellKind::Blank => { /* nothing to show */ } + } + }; + + sub_cell_ui( + ui, + &mut self.expanded_rows, + instance_indices, + sub_cell_content, + ); } } From 45ddb3f11fbedf37fb6c72e0845c74a1d527a633 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 15:43:55 +0200 Subject: [PATCH 10/19] Further method extraction to fight against right drift --- .../src/dataframe_ui.rs | 247 ++++++++++-------- 1 file changed, 134 insertions(+), 113 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 69c40d3b9bf1..0e1495c43de2 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -242,6 +242,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { fn cell_ui(&mut self, ui: &mut egui::Ui, cell: &egui_table::CellInfo) { re_tracing::profile_function!(); + //TODO(ab): paint subcell as well if cell.row_nr % 2 == 1 { // Paint stripes ui.painter() @@ -250,11 +251,6 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { debug_assert!(cell.row_nr < self.num_rows, "Bug in egui_table"); - // best effort to get a proper column auto-sizing behavior - if ui.is_sizing_pass() { - ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); - } - egui::Frame::none() .inner_margin(egui::Margin::symmetric(Self::LEFT_RIGHT_MARGIN, 0.0)) .show(ui, |ui| { @@ -272,6 +268,7 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { } impl DataframeTableDelegate<'_> { + /// Draw the content of a cell. fn cell_ui_impl(&mut self, ui: &mut Ui, cell: &egui_table::CellInfo) { let display_data = match &self.display_data { Ok(display_data) => display_data, @@ -298,6 +295,8 @@ impl DataframeTableDelegate<'_> { let column = &batch.columns()[cell.col_nr]; // compute the latest-at query for this row (used to display tooltips) + + // TODO(ab): this is done for every cell but really should be done only once per row let timestamp = display_data .query_time_column_index .and_then(|col_idx| { @@ -327,121 +326,36 @@ impl DataframeTableDelegate<'_> { .chain((0..instance_count).map(Option::Some)) .take(row_expansion as usize + 1); + // how the sub-cell is drawn let sub_cell_content = |ui: &mut egui::Ui, expanded_rows: &mut ExpandedRows<'_>, sub_cell_index: usize, instance_index: Option| { - /// What kinds of sub-cells might we encounter here? - enum SubcellKind { - /// Summary line with content that as zero or one instances, so cannot be expanded. - Summary, - - /// Summary line with >1 instances, so can be expanded. - SummaryWithExpand, - - /// A particular instance with the corresponding index. - Instance(u64 /* instance index */), - - /// There are more instances than available sub-cells, so this is a summary of how many there are left. - MoreInstancesSummary(u64 /* remaining instances */), - - /// Not enough instances to fill this sub-cell. - Blank, - } - - // The truth table that determines what kind of sub-cell we are dealing with. - let subcell_kind = match instance_index { - // First row with >1 instances. - None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, - - // First row with 0 or 1 instances. - None => SubcellKind::Summary, - - // Last sub-cell and possibly too many instances to display. - Some(instance_index) - if { - sub_cell_index as u64 == row_expansion && instance_index < instance_count - } => - { - let remaining = instance_count - .saturating_sub(instance_index) - .saturating_sub(1); - if remaining > 0 { - // +1 is because the "X more…" line takes one instance spot - SubcellKind::MoreInstancesSummary(remaining + 1) - } else { - SubcellKind::Instance(instance_index) - } - } - - // Some sub-cell for which an instance exists. - Some(instance_index) if { instance_index < instance_count } => { - SubcellKind::Instance(instance_index) - } - - // Some sub-cell for which no instance exists. - Some(_) => SubcellKind::Blank, + // This is called when data actually needs to be drawn (as opposed to summaries like + // "N instances" or "N more…"). + let data_content = |ui: &mut egui::Ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + batch_row_idx, + instance_index, + ); }; - match subcell_kind { - SubcellKind::Summary => { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - batch_row_idx, - instance_index, - ); - } - - SubcellKind::SummaryWithExpand => { - let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{instance_count} instances")); - }); - - if cell_clicked { - if instance_count == row_expansion { - expanded_rows.collapse_row(cell.row_nr); - } else { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); - } - } - } - - SubcellKind::Instance(instance_index) => { - let cell_clicked = - cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - batch_row_idx, - Some(instance_index), - ); - }); - - if cell_clicked { - expanded_rows.collapse_row(cell.row_nr); - } - } - - SubcellKind::MoreInstancesSummary(remaining_instances) => { - let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{remaining_instances} more…")); - }); - - if cell_clicked { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); - } - } - - SubcellKind::Blank => { /* nothing to show */ } - } + sub_cell_ui( + ui, + expanded_rows, + sub_cell_index, + instance_index, + instance_count, + cell, + data_content, + ); }; - sub_cell_ui( + split_ui_vertically( ui, &mut self.expanded_rows, instance_indices, @@ -517,6 +431,113 @@ fn dataframe_ui_impl( }); } +/// Draw a single sub-cell in a table. +/// +/// This deals with the row expansion interaction and logic, as well as summarizing the data when +/// necessary. The actual data drawing is delegated to the `data_content` closure. +#[allow(clippy::too_many_arguments)] +fn sub_cell_ui( + ui: &mut egui::Ui, + expanded_rows: &mut ExpandedRows<'_>, + sub_cell_index: usize, + instance_index: Option, + instance_count: u64, + cell: &egui_table::CellInfo, + data_content: impl Fn(&mut egui::Ui), +) { + let row_expansion = expanded_rows.row_expansion(cell.row_nr); + + /// What kinds of sub-cells might we encounter here? + enum SubcellKind { + /// Summary line with content that as zero or one instances, so cannot be expanded. + Summary, + + /// Summary line with >1 instances, so can be expanded. + SummaryWithExpand, + + /// A particular instance + Instance, + + /// There are more instances than available sub-cells, so this is a summary of how many there are left. + MoreInstancesSummary(u64 /* remaining instances */), + + /// Not enough instances to fill this sub-cell. + Blank, + } + + // The truth table that determines what kind of sub-cell we are dealing with. + let subcell_kind = match instance_index { + // First row with >1 instances. + None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, + + // First row with 0 or 1 instances. + None => SubcellKind::Summary, + + // Last sub-cell and possibly too many instances to display. + Some(instance_index) + if { sub_cell_index as u64 == row_expansion && instance_index < instance_count } => + { + let remaining = instance_count + .saturating_sub(instance_index) + .saturating_sub(1); + if remaining > 0 { + // +1 is because the "X more…" line takes one instance spot + SubcellKind::MoreInstancesSummary(remaining + 1) + } else { + SubcellKind::Instance + } + } + + // Some sub-cell for which an instance exists. + Some(instance_index) if { instance_index < instance_count } => SubcellKind::Instance, + + // Some sub-cell for which no instance exists. + Some(_) => SubcellKind::Blank, + }; + + match subcell_kind { + SubcellKind::Summary => { + data_content(ui); + } + + SubcellKind::SummaryWithExpand => { + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{instance_count} instances")); + }); + + if cell_clicked { + if instance_count == row_expansion { + expanded_rows.collapse_row(cell.row_nr); + } else { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } + } + + SubcellKind::Instance => { + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { + data_content(ui); + }); + + if cell_clicked { + expanded_rows.collapse_row(cell.row_nr); + } + } + + SubcellKind::MoreInstancesSummary(remaining_instances) => { + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { + ui.label(format!("{remaining_instances} more…")); + }); + + if cell_clicked { + expanded_rows.set_row_expansion(cell.row_nr, instance_count); + } + } + + SubcellKind::Blank => { /* nothing to show */ } + } +} + /// Groups column by entity paths. fn column_groups_for_entity( columns: &[ColumnDescriptor], @@ -613,7 +634,7 @@ fn cell_with_hover_button_ui( /// `context`: whatever mutable context is necessary for the `sub_cell_content` /// `sub_cell_data`: the data to be displayed in each sub-cell /// `sub_cell_content`: the function to draw the content of each sub-cell -fn sub_cell_ui( +fn split_ui_vertically( ui: &mut egui::Ui, context: &mut Ctx, sub_cell_data: impl Iterator, From c197bab8046efc821b3790485be8b445336cad9e Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 15:49:23 +0200 Subject: [PATCH 11/19] Minor cleaning --- .../src/dataframe_ui.rs | 137 +++++++++--------- 1 file changed, 69 insertions(+), 68 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 0e1495c43de2..c9593ff0f26d 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -364,73 +364,6 @@ impl DataframeTableDelegate<'_> { } } -/// Display the result of a [`QueryHandle`] in a table. -fn dataframe_ui_impl( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - query_handle: &QueryHandle<'_>, - expanded_rows_cache: &mut ExpandedRowsCache, -) { - re_tracing::profile_function!(); - - // Salt everything against the query expression, as different queries may lead to entirely - // different tables, etc. - let id_salt = egui::Id::new("__dataframe__").with(query_handle.query_expression()); - - let schema = query_handle.schema(); - let (header_groups, header_entity_paths) = column_groups_for_entity(schema); - - let num_rows = query_handle.num_rows(); - - let mut table_delegate = DataframeTableDelegate { - ctx, - query_handle, - schema, - header_entity_paths, - num_rows, - display_data: Err(anyhow::anyhow!( - "No row data, `fetch_columns_and_rows` not called." - )), - expanded_rows: ExpandedRows::new( - ui.ctx().clone(), - ui.make_persistent_id(id_salt).with("expanded_rows"), - expanded_rows_cache, - query_handle.query_expression(), - re_ui::DesignTokens::table_line_height(), - ), - }; - - let num_sticky_cols = schema - .iter() - .take_while(|cd| matches!(cd, ColumnDescriptor::Control(_) | ColumnDescriptor::Time(_))) - .count(); - - egui::Frame::none().inner_margin(5.0).show(ui, |ui| { - egui_table::Table::new() - .id_salt(id_salt) - .columns( - schema - .iter() - .map(|column_descr| { - egui_table::Column::new(200.0) - .resizable(true) - .id(egui::Id::new(column_descr)) - }) - .collect::>(), - ) - .num_sticky_cols(num_sticky_cols) - .headers(vec![ - egui_table::HeaderRow { - height: re_ui::DesignTokens::table_header_height(), - groups: header_groups, - }, - egui_table::HeaderRow::new(re_ui::DesignTokens::table_header_height()), - ]) - .num_rows(num_rows) - .show(ui, &mut table_delegate); - }); -} - /// Draw a single sub-cell in a table. /// /// This deals with the row expansion interaction and logic, as well as summarizing the data when @@ -458,7 +391,8 @@ fn sub_cell_ui( /// A particular instance Instance, - /// There are more instances than available sub-cells, so this is a summary of how many there are left. + /// There are more instances than available sub-cells, so this is a summary of how many + /// there are left. MoreInstancesSummary(u64 /* remaining instances */), /// Not enough instances to fill this sub-cell. @@ -538,6 +472,73 @@ fn sub_cell_ui( } } +/// Display the result of a [`QueryHandle`] in a table. +fn dataframe_ui_impl( + ctx: &ViewerContext<'_>, + ui: &mut egui::Ui, + query_handle: &QueryHandle<'_>, + expanded_rows_cache: &mut ExpandedRowsCache, +) { + re_tracing::profile_function!(); + + // Salt everything against the query expression, as different queries may lead to entirely + // different tables, etc. + let id_salt = egui::Id::new("__dataframe__").with(query_handle.query_expression()); + + let schema = query_handle.schema(); + let (header_groups, header_entity_paths) = column_groups_for_entity(schema); + + let num_rows = query_handle.num_rows(); + + let mut table_delegate = DataframeTableDelegate { + ctx, + query_handle, + schema, + header_entity_paths, + num_rows, + display_data: Err(anyhow::anyhow!( + "No row data, `fetch_columns_and_rows` not called." + )), + expanded_rows: ExpandedRows::new( + ui.ctx().clone(), + ui.make_persistent_id(id_salt).with("expanded_rows"), + expanded_rows_cache, + query_handle.query_expression(), + re_ui::DesignTokens::table_line_height(), + ), + }; + + let num_sticky_cols = schema + .iter() + .take_while(|cd| matches!(cd, ColumnDescriptor::Control(_) | ColumnDescriptor::Time(_))) + .count(); + + egui::Frame::none().inner_margin(5.0).show(ui, |ui| { + egui_table::Table::new() + .id_salt(id_salt) + .columns( + schema + .iter() + .map(|column_descr| { + egui_table::Column::new(200.0) + .resizable(true) + .id(egui::Id::new(column_descr)) + }) + .collect::>(), + ) + .num_sticky_cols(num_sticky_cols) + .headers(vec![ + egui_table::HeaderRow { + height: re_ui::DesignTokens::table_header_height(), + groups: header_groups, + }, + egui_table::HeaderRow::new(re_ui::DesignTokens::table_header_height()), + ]) + .num_rows(num_rows) + .show(ui, &mut table_delegate); + }); +} + /// Groups column by entity paths. fn column_groups_for_entity( columns: &[ColumnDescriptor], From ee804568902f7f66c71a78c94a81d4d8ba87f515 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 16:02:24 +0200 Subject: [PATCH 12/19] Tiny cleanup --- crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index c9593ff0f26d..2749fef78c0c 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -449,9 +449,7 @@ fn sub_cell_ui( } SubcellKind::Instance => { - let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, |ui| { - data_content(ui); - }); + let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, data_content); if cell_clicked { expanded_rows.collapse_row(cell.row_nr); From f5786b8812a703ec31b0f7977b934c0aebd458f9 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 18:41:35 +0200 Subject: [PATCH 13/19] Improve salting, which fixes the refresh bug --- .../src/dataframe_ui.rs | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 2749fef78c0c..9d880f3b3626 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -368,7 +368,6 @@ impl DataframeTableDelegate<'_> { /// /// This deals with the row expansion interaction and logic, as well as summarizing the data when /// necessary. The actual data drawing is delegated to the `data_content` closure. -#[allow(clippy::too_many_arguments)] fn sub_cell_ui( ui: &mut egui::Ui, expanded_rows: &mut ExpandedRows<'_>, @@ -479,11 +478,18 @@ fn dataframe_ui_impl( ) { re_tracing::profile_function!(); - // Salt everything against the query expression, as different queries may lead to entirely - // different tables, etc. - let id_salt = egui::Id::new("__dataframe__").with(query_handle.query_expression()); - let schema = query_handle.schema(); + + // ID salting rationale: + // - The table id mainly drives column widths, so it should be stable across queries leading to + // the same schema. + // - The row expansion is easier to invalidate since it depends on actual row data. For now, we + // use the query expression as salt (but that doesn't cover all edge cases since content may + // vary for a given query as data is ingested). + let table_id_salt = egui::Id::new("__dataframe__").with(schema); + let row_expansion_id_salt = + egui::Id::new("__dataframe__").with(query_handle.query_expression()); + let (header_groups, header_entity_paths) = column_groups_for_entity(schema); let num_rows = query_handle.num_rows(); @@ -499,7 +505,7 @@ fn dataframe_ui_impl( )), expanded_rows: ExpandedRows::new( ui.ctx().clone(), - ui.make_persistent_id(id_salt).with("expanded_rows"), + ui.make_persistent_id(row_expansion_id_salt), expanded_rows_cache, query_handle.query_expression(), re_ui::DesignTokens::table_line_height(), @@ -513,7 +519,7 @@ fn dataframe_ui_impl( egui::Frame::none().inner_margin(5.0).show(ui, |ui| { egui_table::Table::new() - .id_salt(id_salt) + .id_salt(table_id_salt) .columns( schema .iter() From 91d19a6ac4d02f251ad580b4c8fb350b3542fcf0 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 19:08:01 +0200 Subject: [PATCH 14/19] Much improved, simplified, and documented the row expansion cache invalidation --- .../src/dataframe_ui.rs | 38 +++++++------- .../src/expanded_rows.rs | 50 +++++++------------ 2 files changed, 39 insertions(+), 49 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 9d880f3b3626..42f613612ae1 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -13,7 +13,7 @@ use re_ui::UiExt as _; use re_viewer_context::ViewerContext; use crate::display_record_batch::{DisplayRecordBatch, DisplayRecordBatchError}; -use crate::expanded_rows::{ExpandedRows, ExpandedRowsCache, QueryExpression}; +use crate::expanded_rows::{ExpandedRows, ExpandedRowsCache}; /// Display a dataframe table for the provided query. pub(crate) fn dataframe_ui<'a>( @@ -64,13 +64,6 @@ impl QueryHandle<'_> { QueryHandle::Range(query_handle) => query_handle.query().timeline, } } - - fn query_expression(&self) -> QueryExpression { - match self { - QueryHandle::LatestAt(query_handle) => query_handle.query().clone().into(), - QueryHandle::Range(query_handle) => query_handle.query().clone().into(), - } - } } impl<'a> From> for QueryHandle<'a> { @@ -480,15 +473,27 @@ fn dataframe_ui_impl( let schema = query_handle.schema(); - // ID salting rationale: - // - The table id mainly drives column widths, so it should be stable across queries leading to - // the same schema. - // - The row expansion is easier to invalidate since it depends on actual row data. For now, we - // use the query expression as salt (but that doesn't cover all edge cases since content may - // vary for a given query as data is ingested). + // The table id mainly drives column widths, so it should be stable across queries leading to + // the same schema. let table_id_salt = egui::Id::new("__dataframe__").with(schema); - let row_expansion_id_salt = - egui::Id::new("__dataframe__").with(query_handle.query_expression()); + + // It's trickier for the row expansion cache. + // + // For latest-at view, there is always a single row, so it's ok to validate the cache against + // the schema. This means that changing the latest-at time stamp does _not_ invalidate, which is + // desirable. Otherwise, it would be impossible to expand a row when tracking the time panel + // while it is playing. + // + // For range queries, the row layout can change drastically when the query min/max times are + // modified, so in that case we invalidate against the query expression. This means that the + // expanded-ness is reset as soon as the min/max boundaries are changed in the selection panel, + // which is acceptable. + let row_expansion_id_salt = match query_handle { + QueryHandle::LatestAt(_) => egui::Id::new("__dataframe_row_exp__").with(schema), + QueryHandle::Range(query) => egui::Id::new("__dataframe_row_exp__").with(query.query()), + }; + + //egui::Id::new("__dataframe__").with(); let (header_groups, header_entity_paths) = column_groups_for_entity(schema); @@ -507,7 +512,6 @@ fn dataframe_ui_impl( ui.ctx().clone(), ui.make_persistent_id(row_expansion_id_salt), expanded_rows_cache, - query_handle.query_expression(), re_ui::DesignTokens::table_line_height(), ), }; diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index 0a5794b9ffd9..5b2ce40af498 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -1,27 +1,7 @@ use std::collections::BTreeMap; -use re_chunk_store::{LatestAtQueryExpression, RangeQueryExpression}; - -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub(crate) enum QueryExpression { - LatestAt(re_chunk_store::LatestAtQueryExpression), - Range(re_chunk_store::RangeQueryExpression), -} - -impl From for QueryExpression { - fn from(value: LatestAtQueryExpression) -> Self { - Self::LatestAt(value) - } -} - -impl From for QueryExpression { - fn from(value: RangeQueryExpression) -> Self { - Self::Range(value) - } -} - /// Storage for [`ExpandedRows`], which should be persisted across frames. -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone)] pub(crate) struct ExpandedRowsCache { /// Maps "table row number" to "additional expanded rows". /// @@ -29,19 +9,26 @@ pub(crate) struct ExpandedRowsCache { /// used for instances. expanded_rows: BTreeMap, - /// Keep track of the query for which this cache is valid. - // TODO(ab): is there a better invalidation strategy? This doesn't capture the fact that the - // returned data might vary with time (e.g. upon ingestion) - valid_for: Option, + /// ID used to invalidate the cache. + valid_for: egui::Id, +} + +impl Default for ExpandedRowsCache { + fn default() -> Self { + Self { + expanded_rows: BTreeMap::default(), + valid_for: egui::Id::new(""), + } + } } impl ExpandedRowsCache { /// This sets the query used for cache invalidation. /// /// If the query doesn't match the cached one, the state will be reset. - fn set_query(&mut self, query_expression: QueryExpression) { - if Some(&query_expression) != self.valid_for.as_ref() { - self.valid_for = Some(query_expression); + fn validate_id(&mut self, id: egui::Id) { + if id != self.valid_for { + self.valid_for = id; self.expanded_rows = BTreeMap::default(); } } @@ -76,17 +63,16 @@ impl<'a> ExpandedRows<'a> { /// Create a new [`ExpandedRows`] instance. /// /// `egui_ctx` is used to animate the row expansion - /// `id` is used to store the animation state, make it persistent and unique - /// `query_expression` is used to invalidate the cache + /// `id` is used to store the animation state and invalidate the cache, make it persistent and + /// unique pub(crate) fn new( egui_ctx: egui::Context, id: egui::Id, cache: &'a mut ExpandedRowsCache, - query_expression: impl Into, row_height: f32, ) -> Self { // (in-)validate the cache - cache.set_query(query_expression.into()); + cache.validate_id(id); Self { row_height, From 500939ad354507dab25bfb517ef554fcee54102c Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 19:22:20 +0200 Subject: [PATCH 15/19] Replace the test script with a (simpler) zoo specimen --- tests/python/chunk_zoo/chunk_zoo.py | 32 +++++++++++ .../instance_count_test.py | 55 ------------------- 2 files changed, 32 insertions(+), 55 deletions(-) delete mode 100644 tests/python/instance_count_test/instance_count_test.py diff --git a/tests/python/chunk_zoo/chunk_zoo.py b/tests/python/chunk_zoo/chunk_zoo.py index b6e760131c3b..347a406595e0 100644 --- a/tests/python/chunk_zoo/chunk_zoo.py +++ b/tests/python/chunk_zoo/chunk_zoo.py @@ -11,6 +11,7 @@ import argparse from typing import Sequence +import numpy as np import rerun as rr import rerun.components as rrc @@ -114,6 +115,37 @@ def specimen_archetype_without_clamp_join_semantics(): ) +def specimen_many_rows_with_mismatched_instance_count(): + """Points2D across many timestamps with varying and mismatch instance counts.""" + + # Useful for dataframe view row expansion testing. + + np.random.seed(0) + positions_partitions = np.random.randint( + 3, + 15, + size=100, + ) + batch_size = np.sum(positions_partitions) + + # Shuffle the color partitions to induce the mismatch + colors_partitions = positions_partitions.copy() + np.random.shuffle(colors_partitions) + + positions = np.random.rand(batch_size, 2) + colors = np.random.randint(0, 255, size=(batch_size, 4)) + + rr.send_columns( + "/many_rows_with_mismatched_instance_count", + times(range(len(positions_partitions))), + [ + rrc.Position2DBatch(positions).partition(positions_partitions), + rrc.ColorBatch(colors).partition(colors_partitions), + ], + ) + rr.log_components("/many_rows_with_mismatched_instance_count", [rr.Points2D.indicator()], static=True) + + # TODO(ab): add variants (unordered, overlapping timestamps, etc.) def specimen_scalars_interlaced_in_two_chunks(): """Scalar column stored in two chunks, with interlaced timestamps.""" diff --git a/tests/python/instance_count_test/instance_count_test.py b/tests/python/instance_count_test/instance_count_test.py deleted file mode 100644 index 7e84ded3c9da..000000000000 --- a/tests/python/instance_count_test/instance_count_test.py +++ /dev/null @@ -1,55 +0,0 @@ -# Log data with various numbers of instances for test purposes, e.g. UI test, joining, etc. -# TODO(ab): move this to chunk zoo - -from __future__ import annotations - -import argparse -import math -import random - -import numpy as np -import rerun as rr - - -def log_small_point_clouds(seed: int | None = None) -> None: - """Logs a time-varying point cloud with often partial color information.""" - if seed is not None: - random.seed(seed) - np.random.seed(seed) - - N = 1000 - - for i in range(N): - num_points = random.randint(1, 20) - num_colors = random.randint(1, num_points) - x = i / N * 2 - y = math.sin(i / N * 2 * math.pi) - spread = random.random() + 0.2 - - points_x = np.ones(num_points) * x - points_y = np.linspace(y - spread, y + spread, num_points) - - rr.log( - "small_point_cloud/pts", - rr.Points2D( - positions=np.vstack([points_x, points_y]).T, - radii=-4, - colors=np.random.randint(0, 255, size=(num_colors, 4)), - ), - ) - - rr.log("small_point_cloud/bbox", rr.Boxes2D(centers=[[1, 0]], sizes=[[3, 5]]), static=True) - - -def main() -> None: - parser = argparse.ArgumentParser(description="Log some data with varying number of instances for testing purposes") - rr.script_add_args(parser) - args = parser.parse_args() - - rr.script_setup(args, "rerun_example_instance_count_test") - - log_small_point_clouds() - - -if __name__ == "__main__": - main() From 49fef4f574f75c77c84a40f2e75416ce71941db8 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 12 Sep 2024 20:02:39 +0200 Subject: [PATCH 16/19] Only iterate on visible instances based on clip rect (shaved multiple tens of ms frame time) --- .../src/dataframe_ui.rs | 105 +++++++++++------- 1 file changed, 65 insertions(+), 40 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 42f613612ae1..abc9967b48ba 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -263,6 +263,8 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { impl DataframeTableDelegate<'_> { /// Draw the content of a cell. fn cell_ui_impl(&mut self, ui: &mut Ui, cell: &egui_table::CellInfo) { + re_tracing::profile_function!(); + let display_data = match &self.display_data { Ok(display_data) => display_data, Err(err) => { @@ -319,41 +321,46 @@ impl DataframeTableDelegate<'_> { .chain((0..instance_count).map(Option::Some)) .take(row_expansion as usize + 1); - // how the sub-cell is drawn - let sub_cell_content = |ui: &mut egui::Ui, - expanded_rows: &mut ExpandedRows<'_>, - sub_cell_index: usize, - instance_index: Option| { - // This is called when data actually needs to be drawn (as opposed to summaries like - // "N instances" or "N more…"). - let data_content = |ui: &mut egui::Ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - batch_row_idx, - instance_index, - ); - }; - - sub_cell_ui( + { + re_tracing::profile_scope!("subcells"); + + // how the sub-cell is drawn + let sub_cell_content = + |ui: &mut egui::Ui, + expanded_rows: &mut ExpandedRows<'_>, + sub_cell_index: usize, + instance_index: Option| { + // This is called when data actually needs to be drawn (as opposed to summaries like + // "N instances" or "N more…"). + let data_content = |ui: &mut egui::Ui| { + column.data_ui( + self.ctx, + ui, + row_id, + &latest_at_query, + batch_row_idx, + instance_index, + ); + }; + + sub_cell_ui( + ui, + expanded_rows, + sub_cell_index, + instance_index, + instance_count, + cell, + data_content, + ); + }; + + split_ui_vertically( ui, - expanded_rows, - sub_cell_index, - instance_index, - instance_count, - cell, - data_content, + &mut self.expanded_rows, + instance_indices, + sub_cell_content, ); - }; - - split_ui_vertically( - ui, - &mut self.expanded_rows, - instance_indices, - sub_cell_content, - ); + } } } @@ -370,6 +377,8 @@ fn sub_cell_ui( cell: &egui_table::CellInfo, data_content: impl Fn(&mut egui::Ui), ) { + re_tracing::profile_function!(); + let row_expansion = expanded_rows.row_expansion(cell.row_nr); /// What kinds of sub-cells might we encounter here? @@ -649,7 +658,29 @@ fn split_ui_vertically( sub_cell_data: impl Iterator, sub_cell_content: impl Fn(&mut egui::Ui, &mut Ctx, usize, I), ) { - for (sub_cell_index, item_data) in sub_cell_data.enumerate() { + re_tracing::profile_function!(); + + // Empirical testing shows that iterating over all instances can take multiple tens of ms + // when the instance count is very large (which is common). So we use the clip rectangle to + // determine exactly which instances are visible and iterate only over those. + let visible_y_range = ui.clip_rect().y_range(); + let total_y_range = ui.max_rect().y_range(); + + let start_row = ((visible_y_range.min - total_y_range.min) + / re_ui::DesignTokens::table_line_height()) + .at_least(0.0) + .floor() as usize; + + let end_row = ((visible_y_range.max - total_y_range.min) + / re_ui::DesignTokens::table_line_height()) + .at_least(0.0) + .ceil() as usize; + + for (sub_cell_index, item_data) in sub_cell_data + .enumerate() + .skip(start_row) + .take(end_row.saturating_sub(start_row)) + { let sub_cell_rect = egui::Rect::from_min_size( ui.cursor().min + egui::vec2( @@ -668,12 +699,6 @@ fn split_ui_vertically( return; } - if !ui.is_rect_visible(sub_cell_rect) { - continue; - // TODO(ab): detect that we are below any possible screen, in which case we can stop - // entirely. - } - let mut sub_cell_ui = ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); sub_cell_content(&mut sub_cell_ui, context, sub_cell_index, item_data); From 1d76790d8db99df59df5ea4290d1f82708c0013f Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 17 Sep 2024 09:58:35 +0200 Subject: [PATCH 17/19] Review comments --- .typos.toml | 1 + Cargo.lock | 1 + .../viewer/re_space_view_dataframe/Cargo.toml | 1 + .../src/dataframe_ui.rs | 100 +++++++++--------- .../src/expanded_rows.rs | 25 ++--- 5 files changed, 68 insertions(+), 60 deletions(-) diff --git a/.typos.toml b/.typos.toml index 0db77cd3215a..72249872a9f7 100644 --- a/.typos.toml +++ b/.typos.toml @@ -34,6 +34,7 @@ armour = "armor" artefact = "artifact" authorise = "authorize" behaviour = "behavior" +behavioural = "behavioral" British = "American" calibre = "caliber" # allow 'cancelled' since Arrow uses it. diff --git a/Cargo.lock b/Cargo.lock index bce11d2cb0b3..db96128d93d4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5479,6 +5479,7 @@ dependencies = [ "re_data_ui", "re_dataframe", "re_entity_db", + "re_format", "re_log", "re_log_types", "re_renderer", diff --git a/crates/viewer/re_space_view_dataframe/Cargo.toml b/crates/viewer/re_space_view_dataframe/Cargo.toml index 9394d9ebcaa7..11a80b39a69b 100644 --- a/crates/viewer/re_space_view_dataframe/Cargo.toml +++ b/crates/viewer/re_space_view_dataframe/Cargo.toml @@ -23,6 +23,7 @@ re_chunk_store.workspace = true re_data_ui.workspace = true re_dataframe.workspace = true re_entity_db.workspace = true +re_format.workspace = true re_log.workspace = true re_log_types.workspace = true re_renderer.workspace = true diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index abc9967b48ba..a17354f1d06f 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -315,8 +315,10 @@ impl DataframeTableDelegate<'_> { } let instance_count = column.instance_count(batch_row_idx); - let row_expansion = self.expanded_rows.row_expansion(cell.row_nr); + let row_expansion = self.expanded_rows.additional_lines_for_row(cell.row_nr); + // Iterate over the lines for this cell. The initial `None` value corresponds to the summary + // line. let instance_indices = std::iter::once(None) .chain((0..instance_count).map(Option::Some)) .take(row_expansion as usize + 1); @@ -324,7 +326,7 @@ impl DataframeTableDelegate<'_> { { re_tracing::profile_scope!("subcells"); - // how the sub-cell is drawn + // how the line is drawn let sub_cell_content = |ui: &mut egui::Ui, expanded_rows: &mut ExpandedRows<'_>, @@ -364,7 +366,7 @@ impl DataframeTableDelegate<'_> { } } -/// Draw a single sub-cell in a table. +/// Draw a single line in a table. /// /// This deals with the row expansion interaction and logic, as well as summarizing the data when /// necessary. The actual data drawing is delegated to the `data_content` closure. @@ -379,9 +381,9 @@ fn sub_cell_ui( ) { re_tracing::profile_function!(); - let row_expansion = expanded_rows.row_expansion(cell.row_nr); + let row_expansion = expanded_rows.additional_lines_for_row(cell.row_nr); - /// What kinds of sub-cells might we encounter here? + /// What kinds of lines might we encounter here? enum SubcellKind { /// Summary line with content that as zero or one instances, so cannot be expanded. Summary, @@ -392,15 +394,15 @@ fn sub_cell_ui( /// A particular instance Instance, - /// There are more instances than available sub-cells, so this is a summary of how many + /// There are more instances than available lines, so this is a summary of how many /// there are left. - MoreInstancesSummary(u64 /* remaining instances */), + MoreInstancesSummary { remaining_instances: u64 }, - /// Not enough instances to fill this sub-cell. + /// Not enough instances to fill this line. Blank, } - // The truth table that determines what kind of sub-cell we are dealing with. + // The truth table that determines what kind of line we are dealing with. let subcell_kind = match instance_index { // First row with >1 instances. None if { instance_count > 1 } => SubcellKind::SummaryWithExpand, @@ -408,7 +410,7 @@ fn sub_cell_ui( // First row with 0 or 1 instances. None => SubcellKind::Summary, - // Last sub-cell and possibly too many instances to display. + // Last line and possibly too many instances to display. Some(instance_index) if { sub_cell_index as u64 == row_expansion && instance_index < instance_count } => { @@ -417,16 +419,18 @@ fn sub_cell_ui( .saturating_sub(1); if remaining > 0 { // +1 is because the "X more…" line takes one instance spot - SubcellKind::MoreInstancesSummary(remaining + 1) + SubcellKind::MoreInstancesSummary { + remaining_instances: remaining + 1, + } } else { SubcellKind::Instance } } - // Some sub-cell for which an instance exists. + // Some line for which an instance exists. Some(instance_index) if { instance_index < instance_count } => SubcellKind::Instance, - // Some sub-cell for which no instance exists. + // Some line for which no instance exists. Some(_) => SubcellKind::Blank, }; @@ -437,14 +441,17 @@ fn sub_cell_ui( SubcellKind::SummaryWithExpand => { let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{instance_count} instances")); + ui.label(format!( + "{} instances", + re_format::format_uint(instance_count) + )); }); if cell_clicked { if instance_count == row_expansion { - expanded_rows.collapse_row(cell.row_nr); + expanded_rows.remove_additional_lines_for_row(cell.row_nr); } else { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); + expanded_rows.set_additional_lines_for_row(cell.row_nr, instance_count); } } } @@ -453,17 +460,22 @@ fn sub_cell_ui( let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::COLLAPSE, data_content); if cell_clicked { - expanded_rows.collapse_row(cell.row_nr); + expanded_rows.remove_additional_lines_for_row(cell.row_nr); } } - SubcellKind::MoreInstancesSummary(remaining_instances) => { + SubcellKind::MoreInstancesSummary { + remaining_instances, + } => { let cell_clicked = cell_with_hover_button_ui(ui, &re_ui::icons::EXPAND, |ui| { - ui.label(format!("{remaining_instances} more…")); + ui.label(format!( + "{} more…", + re_format::format_uint(remaining_instances) + )); }); if cell_clicked { - expanded_rows.set_row_expansion(cell.row_nr, instance_count); + expanded_rows.set_additional_lines_for_row(cell.row_nr, instance_count); } } @@ -502,8 +514,6 @@ fn dataframe_ui_impl( QueryHandle::Range(query) => egui::Id::new("__dataframe_row_exp__").with(query.query()), }; - //egui::Id::new("__dataframe__").with(); - let (header_groups, header_entity_paths) = column_groups_for_entity(schema); let num_rows = query_handle.num_rows(); @@ -594,7 +604,8 @@ fn error_ui(ui: &mut egui::Ui, error: impl AsRef) { /// /// Returns true if the button was clicked. // TODO(ab, emilk): ideally, egui::Sides should work for that, but it doesn't yet support the -// symmetric behavior (left variable width, right fixed width). +// asymmetric behavior (left variable width, right fixed width). +// See https://github.com/emilk/egui/issues/5116 fn cell_with_hover_button_ui( ui: &mut egui::Ui, icon: &'static re_ui::Icon, @@ -606,14 +617,9 @@ fn cell_with_hover_button_ui( return false; } - let (is_hovering_cell, is_clicked) = ui.input(|i| { - ( - i.pointer - .interact_pos() - .is_some_and(|pos| ui.max_rect().contains(pos)), - i.pointer.button_clicked(egui::PointerButton::Primary), - ) - }); + let is_hovering_cell = ui.rect_contains_pointer(ui.max_rect()); + let is_clicked = + is_hovering_cell && ui.input(|i| i.pointer.button_clicked(egui::PointerButton::Primary)); if is_hovering_cell { let mut content_rect = ui.max_rect(); @@ -647,16 +653,16 @@ fn cell_with_hover_button_ui( } } -/// Helper to draw individual rows (aka sub-cell) into an expanded cell in a table. +/// Helper to draw individual lines into an expanded cell in a table. /// -/// `context`: whatever mutable context is necessary for the `sub_cell_content` -/// `sub_cell_data`: the data to be displayed in each sub-cell -/// `sub_cell_content`: the function to draw the content of each sub-cell -fn split_ui_vertically( +/// `context`: whatever mutable context is necessary for the `line_content_ui` +/// `line_data`: the data to be displayed in each line +/// `line_content_ui`: the function to draw the content of each line +fn split_ui_vertically( ui: &mut egui::Ui, context: &mut Ctx, - sub_cell_data: impl Iterator, - sub_cell_content: impl Fn(&mut egui::Ui, &mut Ctx, usize, I), + line_data: impl Iterator, + line_content_ui: impl Fn(&mut egui::Ui, &mut Ctx, usize, Item), ) { re_tracing::profile_function!(); @@ -666,26 +672,25 @@ fn split_ui_vertically( let visible_y_range = ui.clip_rect().y_range(); let total_y_range = ui.max_rect().y_range(); + // Note: converting float to unsigned ints implicitly saturate negative values to 0 let start_row = ((visible_y_range.min - total_y_range.min) / re_ui::DesignTokens::table_line_height()) - .at_least(0.0) .floor() as usize; let end_row = ((visible_y_range.max - total_y_range.min) / re_ui::DesignTokens::table_line_height()) - .at_least(0.0) .ceil() as usize; - for (sub_cell_index, item_data) in sub_cell_data + for (line_index, item_data) in line_data .enumerate() .skip(start_row) .take(end_row.saturating_sub(start_row)) { - let sub_cell_rect = egui::Rect::from_min_size( + let line_rect = egui::Rect::from_min_size( ui.cursor().min + egui::vec2( 0.0, - sub_cell_index as f32 * re_ui::DesignTokens::table_line_height(), + line_index as f32 * re_ui::DesignTokens::table_line_height(), ), egui::vec2( ui.available_width(), @@ -693,14 +698,13 @@ fn split_ui_vertically( ), ); - // During animation, there may be more sub-cells than can possibly fit. If so, no point in + // During animation, there may be more lines than can possibly fit. If so, no point in // continuing to draw them. - if !ui.max_rect().intersects(sub_cell_rect) { + if !ui.max_rect().intersects(line_rect) { return; } - let mut sub_cell_ui = ui.new_child(egui::UiBuilder::new().max_rect(sub_cell_rect)); - - sub_cell_content(&mut sub_cell_ui, context, sub_cell_index, item_data); + let mut line_ui = ui.new_child(egui::UiBuilder::new().max_rect(line_rect)); + line_content_ui(&mut line_ui, context, line_index, item_data); } } diff --git a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs index 5b2ce40af498..a3523dd81820 100644 --- a/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs +++ b/crates/viewer/re_space_view_dataframe/src/expanded_rows.rs @@ -1,11 +1,14 @@ use std::collections::BTreeMap; /// Storage for [`ExpandedRows`], which should be persisted across frames. +/// +/// Note: each view should store its own cache. Using a [`re_viewer_context::SpaceViewState`] is a +/// good way to do this. #[derive(Debug, Clone)] pub(crate) struct ExpandedRowsCache { - /// Maps "table row number" to "additional expanded rows". + /// Maps "table row number" to "additional lines". /// - /// When expanded, the base space is still used for the summary, which the additional space is + /// When expanded, the base space is still used for the summary, while the additional lines are /// used for instances. expanded_rows: BTreeMap, @@ -87,10 +90,10 @@ impl<'a> ExpandedRows<'a> { self.cache .expanded_rows .range(0..row_nr) - .map(|(expanded_row_nr, expanded)| { + .map(|(expanded_row_nr, additional_lines)| { self.egui_ctx.animate_value_with_time( self.row_id(*expanded_row_nr), - *expanded as f32 * self.row_height, + *additional_lines as f32 * self.row_height, self.egui_ctx.style().animation_time, ) }) @@ -98,15 +101,15 @@ impl<'a> ExpandedRows<'a> { + row_nr as f32 * self.row_height } - /// Return by how much row space this row is expended. - pub(crate) fn row_expansion(&self, row_nr: u64) -> u64 { + /// Return by how many additional lines this row is expended. + pub(crate) fn additional_lines_for_row(&self, row_nr: u64) -> u64 { self.cache.expanded_rows.get(&row_nr).copied().unwrap_or(0) } /// Set the expansion of a row. /// /// Units are in extra row heights. - pub(crate) fn set_row_expansion(&mut self, row_nr: u64, additional_row_space: u64) { + pub(crate) fn set_additional_lines_for_row(&mut self, row_nr: u64, additional_lines: u64) { // Note: don't delete the entry when set to 0, this breaks animation. // If this is the first time this row is expended, we must seed the corresponding animation @@ -119,14 +122,12 @@ impl<'a> ExpandedRows<'a> { ); } - self.cache - .expanded_rows - .insert(row_nr, additional_row_space); + self.cache.expanded_rows.insert(row_nr, additional_lines); } /// Collapse a row. - pub(crate) fn collapse_row(&mut self, row_nr: u64) { - self.set_row_expansion(row_nr, 0); + pub(crate) fn remove_additional_lines_for_row(&mut self, row_nr: u64) { + self.set_additional_lines_for_row(row_nr, 0); } #[inline] From 51716df2b09384efe593813e252586f8c0a36183 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 17 Sep 2024 10:23:56 +0200 Subject: [PATCH 18/19] Some more renaming --- .../src/dataframe_ui.rs | 60 +++++++++---------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index a17354f1d06f..d6c854474e5e 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -327,41 +327,35 @@ impl DataframeTableDelegate<'_> { re_tracing::profile_scope!("subcells"); // how the line is drawn - let sub_cell_content = - |ui: &mut egui::Ui, - expanded_rows: &mut ExpandedRows<'_>, - sub_cell_index: usize, - instance_index: Option| { - // This is called when data actually needs to be drawn (as opposed to summaries like - // "N instances" or "N more…"). - let data_content = |ui: &mut egui::Ui| { - column.data_ui( - self.ctx, - ui, - row_id, - &latest_at_query, - batch_row_idx, - instance_index, - ); - }; - - sub_cell_ui( + let line_content = |ui: &mut egui::Ui, + expanded_rows: &mut ExpandedRows<'_>, + line_index: usize, + instance_index: Option| { + // This is called when data actually needs to be drawn (as opposed to summaries like + // "N instances" or "N more…"). + let data_content = |ui: &mut egui::Ui| { + column.data_ui( + self.ctx, ui, - expanded_rows, - sub_cell_index, + row_id, + &latest_at_query, + batch_row_idx, instance_index, - instance_count, - cell, - data_content, ); }; - split_ui_vertically( - ui, - &mut self.expanded_rows, - instance_indices, - sub_cell_content, - ); + line_ui( + ui, + expanded_rows, + line_index, + instance_index, + instance_count, + cell, + data_content, + ); + }; + + split_ui_vertically(ui, &mut self.expanded_rows, instance_indices, line_content); } } } @@ -370,10 +364,10 @@ impl DataframeTableDelegate<'_> { /// /// This deals with the row expansion interaction and logic, as well as summarizing the data when /// necessary. The actual data drawing is delegated to the `data_content` closure. -fn sub_cell_ui( +fn line_ui( ui: &mut egui::Ui, expanded_rows: &mut ExpandedRows<'_>, - sub_cell_index: usize, + line_index: usize, instance_index: Option, instance_count: u64, cell: &egui_table::CellInfo, @@ -412,7 +406,7 @@ fn sub_cell_ui( // Last line and possibly too many instances to display. Some(instance_index) - if { sub_cell_index as u64 == row_expansion && instance_index < instance_count } => + if { line_index as u64 == row_expansion && instance_index < instance_count } => { let remaining = instance_count .saturating_sub(instance_index) From 89313e576be78fc5744689503aa79bf0a3cbb8a2 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 17 Sep 2024 16:10:55 +0200 Subject: [PATCH 19/19] Updated egui commit to last version (which removed the panic) --- Cargo.lock | 20 +++++++++---------- Cargo.toml | 12 +++++------ .../src/datatype_uis/float_drag.rs | 4 ++-- crates/viewer/re_component_ui/src/range1d.rs | 4 ++-- .../re_component_ui/src/visual_bounds2d.rs | 8 ++++---- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db96128d93d4..f2af43c313f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1777,7 +1777,7 @@ checksum = "68b0cf012f1230e43cd00ebb729c6bb58707ecfa8ad08b52ef3a4ccd2697fc30" [[package]] name = "ecolor" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "bytemuck", "emath", @@ -1787,7 +1787,7 @@ dependencies = [ [[package]] name = "eframe" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "ahash", "bytemuck", @@ -1824,7 +1824,7 @@ dependencies = [ [[package]] name = "egui" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "accesskit", "ahash", @@ -1841,7 +1841,7 @@ dependencies = [ [[package]] name = "egui-wgpu" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "ahash", "bytemuck", @@ -1860,7 +1860,7 @@ dependencies = [ [[package]] name = "egui-winit" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "accesskit_winit", "ahash", @@ -1900,7 +1900,7 @@ dependencies = [ [[package]] name = "egui_extras" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "ahash", "egui", @@ -1916,7 +1916,7 @@ dependencies = [ [[package]] name = "egui_glow" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "ahash", "bytemuck", @@ -1989,7 +1989,7 @@ checksum = "a26ae43d7bcc3b814de94796a5e736d4029efb0ee900c12e2d54c993ad1a1e07" [[package]] name = "emath" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "bytemuck", "serde", @@ -2090,7 +2090,7 @@ dependencies = [ [[package]] name = "epaint" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" dependencies = [ "ab_glyph", "ahash", @@ -2109,7 +2109,7 @@ dependencies = [ [[package]] name = "epaint_default_fonts" version = "0.28.1" -source = "git+https://github.com/emilk/egui.git?rev=66076101e12eee01dec374285521b0bed4ecc40a#66076101e12eee01dec374285521b0bed4ecc40a" +source = "git+https://github.com/emilk/egui.git?rev=1191d9fa86bcb33b57b264b0105f986005f6a6c6#1191d9fa86bcb33b57b264b0105f986005f6a6c6" [[package]] name = "equivalent" diff --git a/Cargo.toml b/Cargo.toml index 66e2f4e619a3..129c6521ade2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -514,12 +514,12 @@ missing_errors_doc = "allow" # As a last resport, patch with a commit to our own repository. # ALWAYS document what PR the commit hash is part of, or when it was merged into the upstream trunk. -ecolor = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 -eframe = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 -egui = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 -egui_extras = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 -egui-wgpu = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 -emath = { git = "https://github.com/emilk/egui.git", rev = "66076101e12eee01dec374285521b0bed4ecc40a" } # egui master 2024-09-06 +ecolor = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 +eframe = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 +egui = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 +egui_extras = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 +egui-wgpu = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 +emath = { git = "https://github.com/emilk/egui.git", rev = "1191d9fa86bcb33b57b264b0105f986005f6a6c6" } # egui master 2024-09-17 # Useful while developing: # ecolor = { path = "../../egui/crates/ecolor" } diff --git a/crates/viewer/re_component_ui/src/datatype_uis/float_drag.rs b/crates/viewer/re_component_ui/src/datatype_uis/float_drag.rs index e75fdcad6e33..e1910e7c8121 100644 --- a/crates/viewer/re_component_ui/src/datatype_uis/float_drag.rs +++ b/crates/viewer/re_component_ui/src/datatype_uis/float_drag.rs @@ -40,7 +40,7 @@ pub fn edit_f32_float_raw_impl( let speed = (value.abs() * 0.01).at_least(0.001); ui.add( egui::DragValue::new(value) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(range) .speed(speed), ) @@ -67,7 +67,7 @@ fn edit_f32_zero_to_one_raw(ui: &mut egui::Ui, value: &mut MaybeMutRef<'_, f32>) if let Some(value) = value.as_mut() { ui.add( egui::DragValue::new(value) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(0.0..=1.0) .speed(0.005) .fixed_decimals(2), diff --git a/crates/viewer/re_component_ui/src/range1d.rs b/crates/viewer/re_component_ui/src/range1d.rs index ce3390b48d68..5c1adef2817e 100644 --- a/crates/viewer/re_component_ui/src/range1d.rs +++ b/crates/viewer/re_component_ui/src/range1d.rs @@ -15,14 +15,14 @@ pub fn edit_range1d( let response_min = ui.add( egui::DragValue::new(min) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(f64::NEG_INFINITY..=*max) .speed(speed), ); ui.label("-"); let response_max = ui.add( egui::DragValue::new(max) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(*min..=f64::INFINITY) .speed(speed), ); diff --git a/crates/viewer/re_component_ui/src/visual_bounds2d.rs b/crates/viewer/re_component_ui/src/visual_bounds2d.rs index 7aec58b1b901..47c1a121e9c1 100644 --- a/crates/viewer/re_component_ui/src/visual_bounds2d.rs +++ b/crates/viewer/re_component_ui/src/visual_bounds2d.rs @@ -56,7 +56,7 @@ fn range_mut_ui(ui: &mut egui::Ui, [start, end]: &mut [f64; 2]) -> egui::Respons ui.horizontal_centered(|ui| { let response_min = ui.add( egui::DragValue::new(start) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(f64::MIN..=*end) .max_decimals(2) .speed(speed), @@ -66,7 +66,7 @@ fn range_mut_ui(ui: &mut egui::Ui, [start, end]: &mut [f64; 2]) -> egui::Respons let response_max = ui.add( egui::DragValue::new(end) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(*start..=f64::MAX) .max_decimals(2) .speed(speed), @@ -94,7 +94,7 @@ pub fn singleline_edit_visual_bounds2d( let response_width = ui.add( egui::DragValue::new(&mut width_edit) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(0.001..=f64::MAX) .max_decimals(1) .speed(speed_func(width)), @@ -102,7 +102,7 @@ pub fn singleline_edit_visual_bounds2d( ui.label("×"); let response_height = ui.add( egui::DragValue::new(&mut height_edit) - .clamp_to_range(false) + .clamp_existing_to_range(false) .range(0.001..=f64::MAX) .max_decimals(1) .speed(speed_func(height)),