Introduce a new component for MarkerShape and use it in SeriesPoint (#…

…5004)

### What
 - Add new MarkerShape component
 - Register a component-editor for it
 - Use it in PointVisualizerSystem / TimeSeries space-view
 - Bump up the radius for the point visualizer system


![image](https://github.com/rerun-io/rerun/assets/3312232/c3f065f4-5bd8-4241-85b8-db4986b5b989)


TODO:
 - #5005


### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5004/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5004/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5004/index.html?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

- [PR Build Summary](https://build.rerun.io/pr/5004)
- [Docs
preview](https://rerun.io/preview/d958ad44008ad21a5313a52985e397b0e04e4d74/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d958ad44008ad21a5313a52985e397b0e04e4d74/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

crates/re_data_ui/src/editors.rs

@@ -5,11 +5,13 @@ use re_data_store::{DataStore, LatestAtQuery};
 use re_log_types::EntityPath;
 use re_query::ComponentWithInstances;
 use re_types::{
-    components::{Color, Radius, ScalarScattering, Text},
+    components::{Color, MarkerShape, Radius, ScalarScattering, Text},
     Component, Loggable,
 };
 use re_viewer_context::{UiVerbosity, ViewerContext};
 
+// ----
+
 #[allow(clippy::too_many_arguments)]
 fn edit_color_ui(
     ctx: &ViewerContext<'_>,
@@ -55,6 +57,8 @@ fn default_color(
     Color::from_rgb(255, 255, 255)
 }
 
+// ----
+
 #[allow(clippy::too_many_arguments)]
 fn edit_text_ui(
     ctx: &ViewerContext<'_>,
@@ -94,6 +98,8 @@ fn default_text(
     Text::from(entity_path.to_string())
 }
 
+// ----
+
 #[allow(clippy::too_many_arguments)]
 fn edit_scatter_ui(
     ctx: &ViewerContext<'_>,
@@ -141,6 +147,8 @@ fn default_scatter(
     ScalarScattering::from(false)
 }
 
+// ----
+
 #[allow(clippy::too_many_arguments)]
 fn edit_radius_ui(
     ctx: &ViewerContext<'_>,
@@ -186,6 +194,55 @@ fn default_radius(
     Radius::from(1.0)
 }
 
+// ----
+
+#[allow(clippy::too_many_arguments)]
+fn edit_marker_shape_ui(
+    ctx: &ViewerContext<'_>,
+    ui: &mut egui::Ui,
+    _verbosity: UiVerbosity,
+    query: &LatestAtQuery,
+    store: &DataStore,
+    entity_path: &EntityPath,
+    override_path: &EntityPath,
+    component: &ComponentWithInstances,
+    instance_key: &re_types::components::InstanceKey,
+) {
+    let current_marker = component
+        .lookup::<MarkerShape>(instance_key)
+        .ok()
+        .unwrap_or_else(|| default_marker_shape(ctx, query, store, entity_path));
+
+    let mut edit_marker = current_marker;
+
+    let marker_text = edit_marker.as_str();
+
+    egui::ComboBox::from_id_source("marker_shape")
+        .selected_text(marker_text)
+        .show_ui(ui, |ui| {
+            ui.style_mut().wrap = Some(false);
+            for marker in MarkerShape::all_markers() {
+                ui.selectable_value(&mut edit_marker, marker, marker.as_str());
+            }
+        });
+
+    if edit_marker != current_marker {
+        ctx.save_blueprint_component(override_path, edit_marker);
+    }
+}
+
+#[inline]
+fn default_marker_shape(
+    _ctx: &ViewerContext<'_>,
+    _query: &LatestAtQuery,
+    _store: &DataStore,
+    _entity_path: &EntityPath,
+) -> MarkerShape {
+    MarkerShape::default()
+}
+
+// ----
+
 fn register_editor<'a, C: Component + Loggable + 'static>(
     registry: &mut re_viewer_context::ComponentUiRegistry,
     default: fn(&ViewerContext<'_>, &LatestAtQuery, &DataStore, &EntityPath) -> C,
@@ -218,4 +275,5 @@ pub fn register_editors(registry: &mut re_viewer_context::ComponentUiRegistry) {
     register_editor::<Text>(registry, default_text, edit_text_ui);
     register_editor::<ScalarScattering>(registry, default_scatter, edit_scatter_ui);
     register_editor::<Radius>(registry, default_radius, edit_radius_ui);
+    register_editor::<MarkerShape>(registry, default_marker_shape, edit_marker_shape_ui);
 }

crates/re_space_view_time_series/src/lib.rs

@@ -11,6 +11,7 @@ mod util;
 mod visualizer_system;
 
 use re_log_types::EntityPath;
+use re_types::components::MarkerShape;
 use re_viewer_context::external::re_entity_db::TimeSeriesAggregator;
 pub use space_view_class::TimeSeriesSpaceView;
 
@@ -36,6 +37,11 @@ pub struct PlotPointAttrs {
     pub kind: PlotSeriesKind,
 }
 
+#[derive(Clone, Copy, Default, Debug, PartialEq, Eq)]
+pub struct ScatterAttrs {
+    pub marker: MarkerShape,
+}
+
 impl PartialEq for PlotPointAttrs {
     fn eq(&self, rhs: &Self) -> bool {
         let Self {
@@ -63,7 +69,7 @@ struct PlotPoint {
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum PlotSeriesKind {
     Continuous,
-    Scatter,
+    Scatter(ScatterAttrs),
     Clear,
 }
 

crates/re_space_view_time_series/src/point_visualizer_system.rs

@@ -1,5 +1,6 @@
 use re_query_cache::{MaybeCachedComponentData, QueryError};
 use re_types::archetypes;
+use re_types::components::MarkerShape;
 use re_types::{
     archetypes::SeriesPoint,
     components::{Color, Radius, Scalar, Text},
@@ -14,6 +15,7 @@ use crate::overrides::initial_override_color;
 use crate::util::{
     determine_plot_bounds_and_time_per_pixel, determine_time_range, points_to_series,
 };
+use crate::ScatterAttrs;
 use crate::{overrides::lookup_override, PlotPoint, PlotPointAttrs, PlotSeries, PlotSeriesKind};
 
 /// The system for rendering [`SeriesPoint`] archetypes.
@@ -29,6 +31,10 @@ impl IdentifiedViewSystem for SeriesPointSystem {
     }
 }
 
+// We use a larger default radius for scatter plots so the marker is
+// visible.
+const DEFAULT_RADIUS: f32 = 3.0;
+
 impl VisualizerSystem for SeriesPointSystem {
     fn visualizer_query_info(&self) -> VisualizerQueryInfo {
         let mut query_info = VisualizerQueryInfo::from_archetype::<archetypes::Scalar>();
@@ -37,6 +43,8 @@ impl VisualizerSystem for SeriesPointSystem {
             .map(ToOwned::to_owned)
             .collect::<ComponentNameSet>();
         query_info.queried.append(&mut series_point_queried);
+        // TODO(jleibs): Use StrokeWidth instead
+        query_info.queried.insert(Radius::name());
         query_info
     }
 
@@ -76,6 +84,8 @@ impl VisualizerSystem for SeriesPointSystem {
     ) -> Option<re_log_types::DataCell> {
         if *component == Color::name() {
             Some([initial_override_color(entity_path)].into())
+        } else if *component == Radius::name() {
+            Some([Radius(DEFAULT_RADIUS)].into())
         } else {
             None
         }
@@ -125,16 +135,18 @@ impl SeriesPointSystem {
 
                 let override_radius = lookup_override::<Radius>(data_result, ctx).map(|r| r.0);
 
+                let override_marker = lookup_override::<MarkerShape>(data_result, ctx);
+
                 let query = re_data_store::RangeQuery::new(query.timeline, time_range);
 
                 // TODO(jleibs): need to do a "joined" archetype query
                 query_caches
-                    .query_archetype_pov1_comp2::<archetypes::Scalar, Scalar, Color, Text, _>(
+                    .query_archetype_pov1_comp3::<archetypes::Scalar, Scalar, Color, MarkerShape, Text, _>(
                         ctx.app_options.experimental_primary_caching_range,
                         store,
                         &query.clone().into(),
                         &data_result.entity_path,
-                        |((time, _row_id), _, scalars, colors, labels)| {
+                        |((time, _row_id), _, scalars, colors, markers, labels)| {
                             let Some(time) = time else {
                                 return;
                             }; // scalars cannot be timeless
@@ -154,12 +166,16 @@ impl SeriesPointSystem {
                                 return;
                             }
 
-                            for (scalar, color, label) in itertools::izip!(
+                            for (scalar, color, marker, label) in itertools::izip!(
                                 scalars.iter(),
                                 MaybeCachedComponentData::iter_or_repeat_opt(
                                     &colors,
                                     scalars.len()
                                 ),
+                                MaybeCachedComponentData::iter_or_repeat_opt(
+                                    &markers,
+                                    scalars.len()
+                                ),
                                 //MaybeCachedComponentData::iter_or_repeat_opt(&radii, scalars.len()),
                                 MaybeCachedComponentData::iter_or_repeat_opt(
                                     &labels,
@@ -178,7 +194,9 @@ impl SeriesPointSystem {
                                 let radius = override_radius
                                     .unwrap_or_else(|| radius.map_or(DEFAULT_RADIUS, |r| r.0));
 
-                                const DEFAULT_RADIUS: f32 = 0.75;
+                                let marker = override_marker.unwrap_or(marker.unwrap_or_default());
+
+
 
                                 points.push(PlotPoint {
                                     time: time.as_i64(),
@@ -187,7 +205,7 @@ impl SeriesPointSystem {
                                         label,
                                         color,
                                         radius,
-                                        kind: PlotSeriesKind::Scatter,
+                                        kind: PlotSeriesKind::Scatter(ScatterAttrs{marker}),
                                     },
                                 });
                             }

crates/re_space_view_time_series/src/space_view_class.rs

@@ -406,30 +406,31 @@ It can greatly improve performance (and readability) in such situations as it pr
                 time_ctrl_write.pause();
             }
 
-            for line in all_plot_series {
-                let points = line
+            for series in all_plot_series {
+                let points = series
                     .points
                     .iter()
                     .map(|p| [(p.0 - time_offset) as _, p.1])
                     .collect::<Vec<_>>();
 
-                let color = line.color;
-                let id = egui::Id::new(line.entity_path.hash());
-                plot_item_id_to_entity_path.insert(id, line.entity_path.clone());
+                let color = series.color;
+                let id = egui::Id::new(series.entity_path.hash());
+                plot_item_id_to_entity_path.insert(id, series.entity_path.clone());
 
-                match line.kind {
+                match series.kind {
                     PlotSeriesKind::Continuous => plot_ui.line(
                         Line::new(points)
-                            .name(&line.label)
+                            .name(&series.label)
                             .color(color)
-                            .width(line.width)
+                            .width(series.width)
                             .id(id),
                     ),
-                    PlotSeriesKind::Scatter => plot_ui.points(
+                    PlotSeriesKind::Scatter(scatter_attrs) => plot_ui.points(
                         Points::new(points)
-                            .name(&line.label)
+                            .name(&series.label)
                             .color(color)
-                            .radius(line.width)
+                            .radius(series.width)
+                            .shape(scatter_attrs.marker.into())
                             .id(id),
                     ),
                     // Break up the chart. At some point we might want something fancier.

crates/re_space_view_time_series/src/util.rs

@@ -3,7 +3,7 @@ use re_viewer_context::{external::re_entity_db::TimeSeriesAggregator, ViewQuery,
 
 use crate::{
     aggregation::{AverageAggregator, MinMaxAggregator},
-    PlotPoint, PlotSeries, PlotSeriesKind,
+    PlotPoint, PlotSeries, PlotSeriesKind, ScatterAttrs,
 };
 
 /// Find the plot bounds and the per-ui-point delta from egui.
@@ -101,7 +101,7 @@ pub fn points_to_series(
         // Can't draw a single point as a continuous line, so fall back on scatter
         let mut kind = points[0].attrs.kind;
         if kind == PlotSeriesKind::Continuous {
-            kind = PlotSeriesKind::Scatter;
+            kind = PlotSeriesKind::Scatter(ScatterAttrs::default());
         }
 
         all_series.push(PlotSeries {

crates/re_space_view_time_series/src/visualizer_system.rs

@@ -12,7 +12,7 @@ use re_viewer_context::{
 use crate::{
     overrides::{initial_override_color, lookup_override},
     util::{determine_plot_bounds_and_time_per_pixel, determine_time_range, points_to_series},
-    PlotPoint, PlotPointAttrs, PlotSeries, PlotSeriesKind,
+    PlotPoint, PlotPointAttrs, PlotSeries, PlotSeriesKind, ScatterAttrs,
 };
 
 /// The legacy system for rendering [`TimeSeriesScalar`] archetypes.
@@ -174,8 +174,8 @@ impl LegacyTimeSeriesSystem {
                             let radius = override_radius
                                 .unwrap_or_else(|| radius.map_or(DEFAULT_RADIUS, |r| r.0));
 
-                            let kind = if scattered {
-                                PlotSeriesKind::Scatter
+                            let kind= if scattered {
+                                PlotSeriesKind::Scatter(ScatterAttrs::default())
                             } else {
                                 PlotSeriesKind::Continuous
                             };

crates/re_types/definitions/rerun/archetypes/series_point.fbs

@@ -18,4 +18,7 @@ table SeriesPoint (
     /// Color for the corresponding series.
     // TODO(jleibs): This should be batch if we make a batch Scalars loggable.
     color: rerun.components.Color ("attr.rerun.component_optional", nullable, order: 1000);
+
+    /// What shape to use to represent the point
+    marker: rerun.components.MarkerShape ("attr.rerun.component_optional", nullable, order: 2000);
 }

crates/re_types/definitions/rerun/components.fbs

@@ -12,6 +12,7 @@ include "./components/instance_key.fbs";
 include "./components/keypoint_id.fbs";
 include "./components/line_strip2d.fbs";
 include "./components/line_strip3d.fbs";
+include "./components/marker_shape.fbs";
 include "./components/material.fbs";
 include "./components/media_type.fbs";
 include "./components/mesh_properties.fbs";

crates/re_types/definitions/rerun/components/marker_shape.fbs

@@ -0,0 +1,33 @@
+include "arrow/attributes.fbs";
+include "python/attributes.fbs";
+include "rust/attributes.fbs";
+
+include "rerun/datatypes.fbs";
+include "rerun/attributes.fbs";
+
+namespace rerun.components;
+
+// TODO(#3384)
+/*
+enum MarkerShape: byte {
+    Circle = 1,
+    Diamond = 2,
+    Square = 3,
+    Cross = 4,
+    Plus = 5,
+    Up = 6,
+    Down = 7,
+    Left = 8,
+    Right = 9,
+    Asterisk = 10,
+}
+*/
+
+/// Shape of a marker.
+struct MarkerShape (
+    "attr.docs.unreleased",
+    "attr.rust.derive": "PartialEq, Eq, PartialOrd, Copy",
+    "attr.rust.repr": "transparent"
+) {
+    shape: ubyte (order: 100);
+}