Skip to content

Commit

Permalink
Improve fallback provider for timeseries Name component (#7203)
Browse files Browse the repository at this point in the history
### What

#7140 improved the handling of timeseries label, but made the
visualisers UI for `Name` not very nice. This PR fixes this.

<img width="476" alt="image"
src="https://github.com/user-attachments/assets/5984b18d-d725-4bba-9fa1-cb9d58a1546e">


### 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/7203?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/7203?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)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7203)
- [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`.
  • Loading branch information
abey79 committed Aug 15, 2024
1 parent 272b107 commit b76d054
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 23 deletions.
5 changes: 1 addition & 4 deletions crates/viewer/re_space_view_time_series/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,7 @@ pub enum PlotSeriesKind {
#[derive(Clone, Debug)]
pub struct PlotSeries {
/// Label of the series.
///
/// If `None`, it means no label was logged or overridden. In this case, it should be derived
/// from the entity path.
pub label: Option<String>,
pub label: String,

pub color: egui::Color32,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,28 @@ impl TypedComponentFallbackProvider<StrokeWidth> for SeriesLineSystem {
}
}

re_viewer_context::impl_component_fallback_provider!(SeriesLineSystem => [Color, StrokeWidth]);
impl TypedComponentFallbackProvider<Name> for SeriesLineSystem {
fn fallback_for(&self, ctx: &QueryContext<'_>) -> Name {
let state = ctx.view_state.downcast_ref::<TimeSeriesSpaceViewState>();

state
.ok()
.and_then(|state| {
state
.default_names_for_entities
.get(ctx.target_entity_path)
.map(|name| name.clone().into())
})
.or_else(|| {
ctx.target_entity_path
.last()
.map(|part| part.ui_string().into())
})
.unwrap_or_default()
}
}

re_viewer_context::impl_component_fallback_provider!(SeriesLineSystem => [Color, StrokeWidth, Name]);

impl SeriesLineSystem {
fn load_scalars(&mut self, ctx: &ViewContext<'_>, query: &ViewQuery<'_>) {
Expand Down Expand Up @@ -367,7 +388,7 @@ impl SeriesLineSystem {
.iter()
.find(|chunk| !chunk.is_empty())
.and_then(|chunk| chunk.component_mono::<Name>(0)?.ok())
.map(|name| name.0.to_string());
.unwrap_or_else(|| self.fallback_for(&query_ctx));

// Now convert the `PlotPoints` into `Vec<PlotSeries>`
let aggregator = results
Expand Down Expand Up @@ -424,7 +445,7 @@ impl SeriesLineSystem {
points,
ctx.recording_store(),
view_query,
series_name,
series_name.into(),
aggregator,
all_series,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,28 @@ impl TypedComponentFallbackProvider<MarkerSize> for SeriesPointSystem {
}
}

re_viewer_context::impl_component_fallback_provider!(SeriesPointSystem => [Color, MarkerSize]);
impl TypedComponentFallbackProvider<Name> for SeriesPointSystem {
fn fallback_for(&self, ctx: &QueryContext<'_>) -> Name {
let state = ctx.view_state.downcast_ref::<TimeSeriesSpaceViewState>();

state
.ok()
.and_then(|state| {
state
.default_names_for_entities
.get(ctx.target_entity_path)
.map(|name| name.clone().into())
})
.or_else(|| {
ctx.target_entity_path
.last()
.map(|part| part.ui_string().into())
})
.unwrap_or_default()
}
}

re_viewer_context::impl_component_fallback_provider!(SeriesPointSystem => [Color, MarkerSize, Name]);

impl SeriesPointSystem {
fn load_scalars(&mut self, ctx: &ViewContext<'_>, query: &ViewQuery<'_>) {
Expand Down Expand Up @@ -446,7 +467,7 @@ impl SeriesPointSystem {
.iter()
.find(|chunk| !chunk.is_empty())
.and_then(|chunk| chunk.component_mono::<Name>(0)?.ok())
.map(|name| name.0.to_string());
.unwrap_or_else(|| self.fallback_for(&query_ctx));

// Now convert the `PlotPoints` into `Vec<PlotSeries>`
points_to_series(
Expand All @@ -455,7 +476,7 @@ impl SeriesPointSystem {
points,
ctx.recording_store(),
view_query,
series_name,
series_name.into(),
// Aggregation for points is not supported.
re_types::components::AggregationPolicy::Off,
all_series,
Expand Down
22 changes: 12 additions & 10 deletions crates/viewer/re_space_view_time_series/src/space_view_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ pub struct TimeSeriesSpaceViewState {
/// Other parts of the system, such as query clamping, need to be aware of that offset in order
/// to work properly.
pub(crate) time_offset: i64,

/// Default names for entities, used when no label is provided.
///
/// This is here because it must be computed with full knowledge of all entities in the plot
/// (e.g. to avoid `hello/x` and `world/x` both being named `x`), and this knowledge must be
/// forwarded to the default providers.
pub(crate) default_names_for_entities: HashMap<EntityPath, String>,
}

impl Default for TimeSeriesSpaceViewState {
Expand All @@ -59,6 +66,7 @@ impl Default for TimeSeriesSpaceViewState {
saved_auto_bounds: Default::default(),
scalar_range: [0.0, 0.0].into(),
time_offset: 0,
default_names_for_entities: Default::default(),
}
}
}
Expand Down Expand Up @@ -427,7 +435,8 @@ Display time series data in a plot.
*state.scalar_range.start_mut() = f64::INFINITY;
*state.scalar_range.end_mut() = f64::NEG_INFINITY;

let label_from_entity = EntityPath::short_names_with_disambiguation(
// Needed by for the visualizers' fallback provider.
state.default_names_for_entities = EntityPath::short_names_with_disambiguation(
all_plot_series
.iter()
.map(|series| series.entity_path.clone()),
Expand All @@ -453,24 +462,17 @@ Display time series data in a plot.
let id = egui::Id::new(series.entity_path.hash());
plot_item_id_to_entity_path.insert(id, series.entity_path.clone());

let label = series
.label
.as_ref()
.or_else(|| label_from_entity.get(&series.entity_path))
.cloned()
.unwrap_or_else(|| "unknown".to_owned());

match series.kind {
PlotSeriesKind::Continuous => plot_ui.line(
Line::new(points)
.name(label)
.name(&series.label)
.color(color)
.width(2.0 * series.radius_ui)
.id(id),
),
PlotSeriesKind::Scatter(scatter_attrs) => plot_ui.points(
Points::new(points)
.name(label)
.name(&series.label)
.color(color)
.radius(series.radius_ui)
.shape(scatter_attrs.marker.into())
Expand Down
7 changes: 4 additions & 3 deletions crates/viewer/re_space_view_time_series/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ pub fn points_to_series(
points: Vec<PlotPoint>,
store: &re_chunk_store::ChunkStore,
query: &ViewQuery<'_>,
series_label: Option<String>,
series_label: String,
aggregator: AggregationPolicy,
all_series: &mut Vec<PlotSeries>,
) {
Expand Down Expand Up @@ -122,7 +122,7 @@ pub fn points_to_series(
});
} else {
add_series_runs(
&series_label,
series_label,
points,
entity_path,
aggregator,
Expand Down Expand Up @@ -199,9 +199,10 @@ pub fn apply_aggregation(
(actual_aggregation_factor, points)
}

#[allow(clippy::needless_pass_by_value)]
#[inline(never)] // Better callstacks on crashes
fn add_series_runs(
series_label: &Option<String>,
series_label: String,
points: Vec<PlotPoint>,
entity_path: &EntityPath,
aggregator: AggregationPolicy,
Expand Down

0 comments on commit b76d054

Please sign in to comment.