From 19efe418f027d49ba2999f11be5c9829acff2893 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Wed, 19 Jun 2024 19:02:36 +0200 Subject: [PATCH] Make object hover & selection colors brighter and more pronounced (#6596) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What #### Problem The outline colors of things that are hovered and/or selected in spatial space views has always been way too dark. Can you quickly spot what is hovered and what is selected in this scene? ![image](https://github.com/rerun-io/rerun/assets/1148717/27e8579b-a5cb-4e88-bbf3-f379bfd54866) The problem with the selection color is that it is the dark blue used for selection of other things, e.g. `ListItem`s: ![image](https://github.com/rerun-io/rerun/assets/1148717/391a8bf2-23cf-4360-aee5-a263ee05f80e) Using the same color for selections everywhere makes sense, except one cannot really use the same color for a thin stroke as one uses for a massive background. #### Solution This PR introduces two new `ContextExt` methods: `hover_stroke` and `selection_stroke`. The hover stroke is very bright, making it directly obvious what is being hovered. The selection stroke is a brighter version of the blue background selection color. The result: ![image](https://github.com/rerun-io/rerun/assets/1148717/b2cd3d6a-991f-4bc5-9c5d-a72ce8893c3d) This is a vast improvement in practice (though the anti-aliasing leaves a little something to be desired…) #### Other I also chose to reuse the same colors for other thing things, like the rays show out in the 2D->3D projection: ![image](https://github.com/rerun-io/rerun/assets/1148717/0d2ea310-6731-4410-bc27-d9f82221ac14) ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6596?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6596?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6596) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --- crates/re_renderer/shader/composite.wgsl | 19 ++++++++++++----- crates/re_space_view_spatial/src/ui.rs | 21 +++++++++++-------- crates/re_space_view_spatial/src/ui_2d.rs | 9 ++++---- crates/re_space_view_spatial/src/ui_3d.rs | 17 ++++++++------- .../src/visualizers/mod.rs | 6 +++++- crates/re_ui/src/context_ext.rs | 18 ++++++++++++++++ crates/re_ui/src/design_tokens.rs | 5 +++++ crates/re_ui/src/list_item/list_item.rs | 11 +++++----- 8 files changed, 73 insertions(+), 33 deletions(-) diff --git a/crates/re_renderer/shader/composite.wgsl b/crates/re_renderer/shader/composite.wgsl index a0d3136efa37..0ac740eb533f 100644 --- a/crates/re_renderer/shader/composite.wgsl +++ b/crates/re_renderer/shader/composite.wgsl @@ -26,8 +26,6 @@ fn main(in: FragmentInput) -> @location(0) vec4f { // The issue is that positions provided by @builtin(position) are not dependent on the set viewport, // but are about the location of the texel in the target texture. var color = textureSample(color_texture, nearest_sampler, in.texcoord).rgb; - // TODO(andreas): Do something meaningful with values above 1 - color = clamp(color, vec3f(0.0), vec3f(1.0)); // Outlines { @@ -43,9 +41,18 @@ fn main(in: FragmentInput) -> @location(0) vec4f { let outline_color_a = outline_a * uniforms.outline_color_layer_a; let outline_color_b = outline_b * uniforms.outline_color_layer_b; - // Blend outlines with screen color. - color = color * (1.0 - outline_color_a.a) + outline_color_a.rgb; - color = color * (1.0 - outline_color_b.a) + outline_color_b.rgb; + // Blend outlines with screen color: + if false { + // Normal blending with premul alpha. + // Problem: things that are both hovered and selected will get double outlines, + // which can look really ugly if e.g. the selection is dark blue and the hover is bright white. + color = color * (1.0 - outline_color_a.a) + outline_color_a.rgb; + color = color * (1.0 - outline_color_b.a) + outline_color_b.rgb; + } else { + // Add the two outline colors, then blend that in: + let outline_color_sum = saturate(outline_color_a + outline_color_b); + color = color * (1.0 - outline_color_sum.a) + outline_color_sum.rgb; + } // Show only the outline. Useful for debugging. //color = outline_color_a.rgb; @@ -54,6 +61,8 @@ fn main(in: FragmentInput) -> @location(0) vec4f { //color = vec3f(closest_positions.xy / resolution, 0.0); } + color = saturate(color); // TODO(andreas): Do something meaningful with values above 1 + // Apply srgb gamma curve - this is necessary since the final eframe output does *not* have an srgb format. return vec4f(srgb_from_linear(color), 1.0); } diff --git a/crates/re_space_view_spatial/src/ui.rs b/crates/re_space_view_spatial/src/ui.rs index 2ff740172ff1..f82528619180 100644 --- a/crates/re_space_view_spatial/src/ui.rs +++ b/crates/re_space_view_spatial/src/ui.rs @@ -17,7 +17,7 @@ use re_types::{ tensor_data::TensorDataMeaning, Loggable as _, }; -use re_ui::UiExt as _; +use re_ui::{ContextExt as _, UiExt as _}; use re_viewer_context::{ HoverHighlight, Item, ItemSpaceContext, SelectionHighlight, SpaceViewHighlights, SpaceViewState, SpaceViewSystemExecutionError, TensorDecodeCache, TensorStatsCache, UiLayout, @@ -368,16 +368,19 @@ pub fn create_labels( } pub fn outline_config(gui_ctx: &egui::Context) -> OutlineConfig { - // Take the exact same colors we have in the ui! - let selection_outline_color = - re_renderer::Rgba::from(gui_ctx.style().visuals.selection.bg_fill); - let hover_outline_color = - re_renderer::Rgba::from(gui_ctx.style().visuals.widgets.hovered.bg_fill); + // Use the exact same colors we have in the ui! + let hover_outline = gui_ctx.hover_stroke(); + let selection_outline = gui_ctx.selection_stroke(); + + // See also: SIZE_BOOST_IN_POINTS_FOR_LINE_OUTLINES + + let outline_radius_ui_pts = 0.5 * f32::max(hover_outline.width, selection_outline.width); + let outline_radius_pixel = (gui_ctx.pixels_per_point() * outline_radius_ui_pts).at_least(0.5); OutlineConfig { - outline_radius_pixel: (gui_ctx.pixels_per_point() * 1.5).at_least(0.5), - color_layer_a: hover_outline_color, - color_layer_b: selection_outline_color, + outline_radius_pixel, + color_layer_a: re_renderer::Rgba::from(hover_outline.color), + color_layer_b: re_renderer::Rgba::from(selection_outline.color), } } diff --git a/crates/re_space_view_spatial/src/ui_2d.rs b/crates/re_space_view_spatial/src/ui_2d.rs index 7560521426eb..36504ac614fa 100644 --- a/crates/re_space_view_spatial/src/ui_2d.rs +++ b/crates/re_space_view_spatial/src/ui_2d.rs @@ -13,6 +13,7 @@ use re_types::{ }, components::ViewCoordinates, }; +use re_ui::ContextExt as _; use re_viewer_context::{ gpu_bridge, ItemSpaceContext, SpaceViewId, SpaceViewSystemExecutionError, SystemExecutionOutput, ViewQuery, ViewerContext, @@ -274,7 +275,7 @@ impl SpatialSpaceView2D { query.space_origin, &ui_from_scene, selected_context, - ui.style().visuals.selection.bg_fill, + ui.ctx().selection_stroke().color, )); } if let Some(hovered_context) = ctx.selection_state().hovered_space_context() { @@ -283,7 +284,7 @@ impl SpatialSpaceView2D { query.space_origin, &ui_from_scene, hovered_context, - egui::Color32::WHITE, + ui.ctx().hover_stroke().color, )); } @@ -426,7 +427,7 @@ fn show_projections_from_3d_space( space: &EntityPath, ui_from_scene: &RectTransform, space_context: &ItemSpaceContext, - color: egui::Color32, + circle_fill_color: egui::Color32, ) -> Vec { let mut shapes = Vec::new(); if let ItemSpaceContext::ThreeD { @@ -445,7 +446,7 @@ fn show_projections_from_3d_space( radius + 2.0, Color32::BLACK, )); - shapes.push(Shape::circle_filled(pos_in_ui, radius, color)); + shapes.push(Shape::circle_filled(pos_in_ui, radius, circle_fill_color)); let text_color = Color32::WHITE; let text = format!("Depth: {:.3} m", pos_2d.z); diff --git a/crates/re_space_view_spatial/src/ui_3d.rs b/crates/re_space_view_spatial/src/ui_3d.rs index 388650079c85..2729ee3e8934 100644 --- a/crates/re_space_view_spatial/src/ui_3d.rs +++ b/crates/re_space_view_spatial/src/ui_3d.rs @@ -1,6 +1,7 @@ use egui::{emath::RectTransform, NumExt as _}; use glam::Affine3A; use macaw::{BoundingBox, Quat, Vec3}; +use re_ui::ContextExt; use re_viewport_blueprint::ViewProperty; use web_time::Instant; @@ -634,7 +635,7 @@ impl SpatialSpaceView3D { space_cameras, state, selected_context, - ui.style().visuals.selection.bg_fill, + ui.ctx().selection_stroke().color, ); } if let Some(hovered_context) = ctx.selection_state().hovered_space_context() { @@ -643,7 +644,7 @@ impl SpatialSpaceView3D { space_cameras, state, hovered_context, - egui::Color32::WHITE, + ui.ctx().hover_stroke().color, ); } @@ -809,7 +810,7 @@ fn show_projections_from_2d_space( space_cameras: &[SpaceCamera3D], state: &SpatialSpaceViewState, space_context: &ItemSpaceContext, - color: egui::Color32, + ray_color: egui::Color32, ) { match space_context { ItemSpaceContext::TwoD { space_2d, pos } => { @@ -842,7 +843,7 @@ fn show_projections_from_2d_space( ray, &state.bounding_boxes.accumulated, thick_ray_length, - color, + ray_color, ); } } @@ -870,7 +871,7 @@ fn show_projections_from_2d_space( ray, &state.bounding_boxes.accumulated, distance, - color, + ray_color, ); } } @@ -884,7 +885,7 @@ fn add_picking_ray( ray: macaw::Ray3, scene_bbox_accum: &BoundingBox, thick_ray_length: f32, - color: egui::Color32, + ray_color: egui::Color32, ) { let mut line_batch = line_builder.batch("picking ray"); @@ -895,11 +896,11 @@ fn add_picking_ray( line_batch .add_segment(origin, main_ray_end) - .color(color) + .color(ray_color) .radius(Size::new_points(1.0)); line_batch .add_segment(main_ray_end, fallback_ray_end) - .color(color.gamma_multiply(0.7)) + .color(ray_color.gamma_multiply(0.7)) // TODO(andreas): Make this dashed. .radius(Size::new_points(0.5)); } diff --git a/crates/re_space_view_spatial/src/visualizers/mod.rs b/crates/re_space_view_spatial/src/visualizers/mod.rs index 71607564b50c..805800fe6ad3 100644 --- a/crates/re_space_view_spatial/src/visualizers/mod.rs +++ b/crates/re_space_view_spatial/src/visualizers/mod.rs @@ -45,7 +45,11 @@ use super::contexts::SpatialSceneEntityContext; pub type Keypoints = HashMap<(re_types::components::ClassId, i64), HashMap>; // TODO(andreas): It would be nice if these wouldn't need to be set on every single line/point builder. -pub const SIZE_BOOST_IN_POINTS_FOR_LINE_OUTLINES: f32 = 1.5; + +/// Gap between lines and their outline. +pub const SIZE_BOOST_IN_POINTS_FOR_LINE_OUTLINES: f32 = 1.0; + +/// Gap between points and their outline. pub const SIZE_BOOST_IN_POINTS_FOR_POINT_OUTLINES: f32 = 2.5; pub fn register_2d_spatial_visualizers( diff --git a/crates/re_ui/src/context_ext.rs b/crates/re_ui/src/context_ext.rs index ab8ac910864a..b4f770875252 100644 --- a/crates/re_ui/src/context_ext.rs +++ b/crates/re_ui/src/context_ext.rs @@ -35,6 +35,24 @@ pub trait ContextExt { } } + /// Hovered UI and spatial primitives should have this outline. + fn hover_stroke(&self) -> egui::Stroke { + // We want something bright here. + self.ctx().style().visuals.widgets.active.fg_stroke + } + + /// Selected UI and spatial primitives should have this outline. + fn selection_stroke(&self) -> egui::Stroke { + self.ctx().style().visuals.selection.stroke + + // It is tempting to use the background selection color for outlines, + // but in practice it is way too dark for spatial views (you can't tell what is selected). + // Also: background colors should not be used as stroke colors. + // let color = self.ctx().style().visuals.selection.bg_fill; + // let stroke_width = self.ctx().style().visuals.selection.stroke.width; + // egui::Stroke::new(stroke_width, color) + } + #[must_use] fn warning_text(&self, text: impl Into) -> egui::RichText { let style = self.ctx().style(); diff --git a/crates/re_ui/src/design_tokens.rs b/crates/re_ui/src/design_tokens.rs index 5dca7c73ad80..e5c4f49998ca 100644 --- a/crates/re_ui/src/design_tokens.rs +++ b/crates/re_ui/src/design_tokens.rs @@ -160,6 +160,7 @@ impl DesignTokens { egui_style.visuals.selection.bg_fill = get_aliased_color(&self.json, "{Alias.Color.Highlight.Default.value}"); + egui_style.visuals.selection.stroke.color = egui::Color32::from_rgb(173, 184, 255); // Brighter version of the above egui_style.visuals.widgets.noninteractive.bg_stroke.color = Color32::from_gray(30); // from figma. separator lines, panel lines, etc @@ -171,6 +172,10 @@ impl DesignTokens { egui_style.visuals.widgets.inactive.fg_stroke.color = default; // button text egui_style.visuals.widgets.active.fg_stroke.color = strong; // strong text and active button text + let wide_stroke_width = 1.5; // Make it a bit more visible, especially important for spatial primitives. + egui_style.visuals.widgets.active.fg_stroke.width = wide_stroke_width; + egui_style.visuals.selection.stroke.width = wide_stroke_width; + // From figma let shadow = egui::epaint::Shadow { offset: egui::vec2(0.0, 15.0), diff --git a/crates/re_ui/src/list_item/list_item.rs b/crates/re_ui/src/list_item/list_item.rs index ce6c0d6fa616..5b20df918b59 100644 --- a/crates/re_ui/src/list_item/list_item.rs +++ b/crates/re_ui/src/list_item/list_item.rs @@ -2,7 +2,10 @@ use egui::{NumExt, Response, Shape, Ui}; -use crate::list_item::{ContentContext, DesiredWidth, LayoutInfoStack, ListItemContent}; +use crate::{ + list_item::{ContentContext, DesiredWidth, LayoutInfoStack, ListItemContent}, + ContextExt as _, +}; use crate::{DesignTokens, UiExt as _}; struct ListItemResponse { @@ -331,11 +334,7 @@ impl ListItem { if drag_target { ui.painter().set( background_frame, - Shape::rect_stroke( - bg_rect_to_paint, - 0.0, - (1.0, ui.visuals().selection.bg_fill), - ), + Shape::rect_stroke(bg_rect_to_paint, 0.0, ui.ctx().hover_stroke()), ); } else if interactive { let bg_fill = if !response.hovered() && ui.rect_contains_pointer(bg_rect) {