Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve UI of various components in the selection panel #6297

Merged
merged 11 commits into from
May 14, 2024
35 changes: 21 additions & 14 deletions crates/re_data_ui/src/annotation_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use re_types::datatypes::{
};
use re_viewer_context::{auto_color, UiLayout, ViewerContext};

use super::{table_for_ui_layout, DataUi};
use super::{label_for_ui_layout, table_for_ui_layout, DataUi};

impl crate::EntityDataUi for re_types::components::ClassId {
fn entity_data_ui(
Expand All @@ -27,10 +27,12 @@ impl crate::EntityDataUi for re_types::components::ClassId {
let response = ui.horizontal(|ui| {
// Color first, to keep subsequent rows of the same things aligned
small_color_ui(ui, &class.info);
ui.label(format!("{}", self.0));
let mut text = format!("{}", self.0);
if let Some(label) = &class.info.label {
ui.label(label.as_str());
text.push_str(" ");
text.push_str(label.as_str());
}
label_for_ui_layout(ui, ui_layout, text);
});

let id = self.0;
Expand All @@ -40,7 +42,7 @@ impl crate::EntityDataUi for re_types::components::ClassId {
|| !class.keypoint_annotations.is_empty()
{
response.response.on_hover_ui(|ui| {
class_description_ui(ctx, ui, ui_layout, class, id);
class_description_ui(ctx, ui, UiLayout::Tooltip, class, id);
});
}
}
Expand All @@ -52,7 +54,7 @@ impl crate::EntityDataUi for re_types::components::ClassId {
}
}
} else {
ui.label(format!("{}", self.0));
label_for_ui_layout(ui, ui_layout, format!("{}", self.0));
}
}
}
Expand All @@ -62,7 +64,7 @@ impl crate::EntityDataUi for re_types::components::KeypointId {
&self,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
entity_path: &re_log_types::EntityPath,
query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
Expand All @@ -71,13 +73,16 @@ impl crate::EntityDataUi for re_types::components::KeypointId {
ui.horizontal(|ui| {
// Color first, to keep subsequent rows of the same things aligned
small_color_ui(ui, &info);
ui.label(format!("{}", self.0));
let mut text = format!("{}", self.0);
if let Some(label) = &info.label {
ui.label(label.as_str());
text.push_str(" ");
text.push_str(label.as_str());
}

label_for_ui_layout(ui, ui_layout, text);
});
} else {
ui.label(format!("{}", self.0));
label_for_ui_layout(ui, ui_layout, format!("{}", self.0));
}
}
}
Expand Down Expand Up @@ -108,16 +113,18 @@ impl DataUi for AnnotationContext {
) {
match ui_layout {
UiLayout::List | UiLayout::Tooltip => {
if self.0.len() == 1 {
let text = if self.0.len() == 1 {
let descr = &self.0[0].class_description;
ui.label(format!(

format!(
"One class containing {} keypoints and {} connections",
descr.keypoint_annotations.len(),
descr.keypoint_connections.len()
));
)
} else {
ui.label(format!("{} classes", self.0.len()));
}
format!("{} classes", self.0.len())
};
label_for_ui_layout(ui, ui_layout, text);
}
UiLayout::SelectionPanelLimitHeight | UiLayout::SelectionPanelFull => {
ui.vertical(|ui| {
Expand Down
2 changes: 2 additions & 0 deletions crates/re_data_ui/src/component_ui_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ pub fn create_component_ui_registry() -> ComponentUiRegistry {
add_to_registry::<re_types::components::KeypointId>(&mut registry);
add_to_registry::<re_types::components::LineStrip2D>(&mut registry);
add_to_registry::<re_types::components::LineStrip3D>(&mut registry);
add_to_registry::<re_types::components::Range1D>(&mut registry);
add_to_registry::<re_types::components::Range2D>(&mut registry);
add_to_registry::<re_types::components::Resolution>(&mut registry);
add_to_registry::<re_types::components::Rotation3D>(&mut registry);
add_to_registry::<re_types::components::Material>(&mut registry);
Expand Down
66 changes: 47 additions & 19 deletions crates/re_data_ui/src/data.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use egui::Vec2;
use egui::{Ui, Vec2};
use re_data_store::LatestAtQuery;
use re_entity_db::EntityDb;

use re_format::format_f32;
use re_types::components::{Color, LineStrip2D, LineStrip3D, ViewCoordinates};
use re_types::components::{Color, LineStrip2D, LineStrip3D, Range1D, Range2D, ViewCoordinates};
use re_viewer_context::{UiLayout, ViewerContext};

use super::{table_for_ui_layout, DataUi};
use super::{label_for_ui_layout, table_for_ui_layout, DataUi};

/// Default number of ui points to show a number.
const DEFAULT_NUMBER_WIDTH: f32 = 52.0;
Expand Down Expand Up @@ -62,13 +64,13 @@ impl DataUi for ViewCoordinates {
) {
match ui_layout {
UiLayout::List => {
ui.label(self.describe_short())
label_for_ui_layout(ui, ui_layout, self.describe_short())
.on_hover_text(self.describe());
}
UiLayout::SelectionPanelFull
| UiLayout::SelectionPanelLimitHeight
| UiLayout::Tooltip => {
ui.label(self.describe());
label_for_ui_layout(ui, ui_layout, self.describe());
}
}
}
Expand Down Expand Up @@ -107,11 +109,11 @@ impl DataUi for re_types::datatypes::Vec2D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Expand All @@ -120,11 +122,11 @@ impl DataUi for re_types::datatypes::Vec3D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Expand All @@ -133,11 +135,11 @@ impl DataUi for re_types::datatypes::Vec4D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Expand All @@ -146,11 +148,11 @@ impl DataUi for re_types::datatypes::UVec2D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Expand All @@ -159,11 +161,11 @@ impl DataUi for re_types::datatypes::UVec3D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Expand All @@ -172,11 +174,37 @@ impl DataUi for re_types::datatypes::UVec4D {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
ui.label(self.to_string());
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

impl DataUi for Range1D {
fn data_ui(
&self,
_ctx: &ViewerContext<'_>,
ui: &mut Ui,
ui_layout: UiLayout,
_query: &LatestAtQuery,
_db: &EntityDb,
) {
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

impl DataUi for Range2D {
fn data_ui(
&self,
_ctx: &ViewerContext<'_>,
ui: &mut Ui,
ui_layout: UiLayout,
_query: &LatestAtQuery,
_db: &EntityDb,
) {
label_for_ui_layout(ui, ui_layout, self.to_string());
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so before we had data ui implement for those we'd just generate a label automatically. Shouldn't that label generation use label_for_ui_layout instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to text_ui() (in component_ui_registry.rs)? I believe several code path lead to that function.

Looking at it a bit more... It's essentially doing a more fancy version of label_for_ui_layout, so these are possibly duplicates. text_ui uses monospace font though, I'm not sure why. Seems like some further cleaning is in order around here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See update PR description. There is much work to be done here still.

Expand All @@ -191,7 +219,7 @@ impl DataUi for LineStrip2D {
) {
match ui_layout {
UiLayout::List | UiLayout::Tooltip => {
ui.label(format!("{} positions", self.0.len()));
label_for_ui_layout(ui, ui_layout, format!("{} positions", self.0.len()));
}
UiLayout::SelectionPanelLimitHeight | UiLayout::SelectionPanelFull => {
use egui_extras::Column;
Expand Down Expand Up @@ -238,7 +266,7 @@ impl DataUi for LineStrip3D {
) {
match ui_layout {
UiLayout::List | UiLayout::Tooltip => {
ui.label(format!("{} positions", self.0.len()));
label_for_ui_layout(ui, ui_layout, format!("{} positions", self.0.len()));
}
UiLayout::SelectionPanelFull | UiLayout::SelectionPanelLimitHeight => {
use egui_extras::Column;
Expand Down
15 changes: 7 additions & 8 deletions crates/re_data_ui/src/image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,17 @@ use re_viewer_context::{
ViewerContext,
};

use crate::image_meaning_for_entity;
use crate::{image_meaning_for_entity, label_for_ui_layout};

use super::EntityDataUi;

pub fn format_tensor_shape_single_line(shape: &[TensorDimension]) -> String {
const MAX_SHOWN: usize = 4; // should be enough for width/height/depth and then some!
let shapes = shape.iter().take(MAX_SHOWN).join(", ");
if shape.len() > MAX_SHOWN {
format!("{shapes}…")
} else {
shapes
}
format!(
"shape: {shapes}{}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shape: part isn't useful and just takes space imho. Interestingly, on the screenshots you took one has it and one doesn't ;-)
(I checked locally, it's always there now)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, one screenshot slept through outdated :)

That said: I dont think 4, 3, 5, 1 by itself is very descriptive. that looks very much like a 1x4 int tensor (which it is not).

I guess I could go 4x3x5x1, but still need to special case when dimension have labels. Will give it a second look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out a decent way, see updated screenshots. I also added some cases to the checklist.

if shape.len() > MAX_SHOWN { ",…" } else { "" }
)
}

impl EntityDataUi for re_types::components::TensorData {
Expand Down Expand Up @@ -64,7 +63,7 @@ impl EntityDataUi for re_types::components::TensorData {
);
}
Err(err) => {
ui.label(ctx.re_ui.error_text(err.to_string()));
label_for_ui_layout(ui, ui_layout, ctx.re_ui.error_text(err.to_string()));
}
}
}
Expand Down Expand Up @@ -157,7 +156,7 @@ pub fn tensor_ui(
],
None => tensor.shape.clone(),
};
ui.add(egui::Label::new(format_tensor_shape_single_line(&shape)).wrap(true))
label_for_ui_layout(ui, ui_layout, format_tensor_shape_single_line(&shape))
.on_hover_ui(|ui| {
tensor_summary_ui(
ctx.re_ui,
Expand Down
17 changes: 17 additions & 0 deletions crates/re_data_ui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,20 @@ pub fn table_for_ui_layout(
}
}
}

pub fn label_for_ui_layout(
ui: &mut egui::Ui,
ui_layout: UiLayout,
text: impl Into<egui::WidgetText>,
) -> egui::Response {
let mut label = egui::Label::new(text);

match ui_layout {
UiLayout::List => label = label.truncate(true),
UiLayout::Tooltip | UiLayout::SelectionPanelLimitHeight | UiLayout::SelectionPanelFull => {
label = label.wrap(true)
}
}

ui.add(label)
}
44 changes: 26 additions & 18 deletions crates/re_data_ui/src/pinhole.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use re_types::components::{PinholeProjection, Resolution};
use re_viewer_context::{UiLayout, ViewerContext};

use crate::DataUi;
use crate::{label_for_ui_layout, DataUi};

impl DataUi for PinholeProjection {
fn data_ui(
Expand All @@ -12,24 +12,32 @@ impl DataUi for PinholeProjection {
query: &re_data_store::LatestAtQuery,
db: &re_entity_db::EntityDb,
) {
if ui_layout == UiLayout::List {
// See if this is a trivial pinhole, and can be displayed as such:
let fl = self.focal_length_in_pixels();
let pp = self.principal_point();
if *self == Self::from_focal_length_and_principal_point(fl, pp) {
let fl = if fl.x() == fl.y() {
fl.x().to_string()
} else {
fl.to_string()
};
match ui_layout {
UiLayout::List => {
// See if this is a trivial pinhole, and can be displayed as such:
let fl = self.focal_length_in_pixels();
let pp = self.principal_point();
if *self == Self::from_focal_length_and_principal_point(fl, pp) {
let fl = if fl.x() == fl.y() {
fl.x().to_string()
} else {
fl.to_string()
};

ui.label(format!("Focal length: {fl}\nPrincipal point: {pp}"))
.on_hover_ui(|ui| self.data_ui(ctx, ui, UiLayout::Tooltip, query, db));
return;
label_for_ui_layout(
ui,
ui_layout,
format!("Focal length: {fl}, principal point: {pp}"),
)
} else {
label_for_ui_layout(ui, ui_layout, "3x3 projection matrix")
}
.on_hover_ui(|ui| self.data_ui(ctx, ui, UiLayout::Tooltip, query, db));
}
_ => {
self.0.data_ui(ctx, ui, ui_layout, query, db);
}
}

self.0.data_ui(ctx, ui, ui_layout, query, db);
}
}

Expand All @@ -38,11 +46,11 @@ impl DataUi for Resolution {
&self,
_ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
_ui_layout: UiLayout,
ui_layout: UiLayout,
_query: &re_data_store::LatestAtQuery,
_db: &re_entity_db::EntityDb,
) {
let [x, y] = self.0 .0;
ui.monospace(format!("{x}x{y}"));
label_for_ui_layout(ui, ui_layout, format!("{x}x{y}"));
}
}
Loading
Loading