Skip to content

Commit

Permalink
Disambiguate plot labels with multiple entities ending with the same …
Browse files Browse the repository at this point in the history
…part (#7140)

### What

Timeseries views defaults series name to the last part of the
corresponding entity (unless a series label is logged/overridden). When
multiple entities with the same ending would be included, the legend
would list that label only once and with a gray color (see before/after
screenshot below).

This PR fixes this by using more entity parts for disambiguation.

Note: to do this, the entity to label conversion has to happen at a
later stage (ie in the view, not the system), which means that we no
longer use the fallback mechanism. This affects the UI in a way that is
IMO acceptable:

<img width="298" alt="image"
src="https://github.com/user-attachments/assets/e0024ed9-6ee7-451a-8d19-ee5f9d640fe5">
<br/><br/>

The default fallback is now used, which displays `<name>`. Of course,
what ends up being displayed as the (disambiguated) entity name. I can't
think of a non-headache way of fixing this, so I'm inclined to let it
be.


### Before/after

<img width="122" alt="image"
src="https://github.com/user-attachments/assets/1d33aa58-b21d-4077-b286-680db2e41cf3">
<img width="171" alt="image"
src="https://github.com/user-attachments/assets/47053a1e-a0b0-4bc0-9b90-0c7db1c2a921">


### 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/7140?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/7140?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/7140)
- [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 12, 2024
1 parent 14eca80 commit cd951dd
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 37 deletions.
113 changes: 113 additions & 0 deletions crates/store/re_log_types/src/path/entity_path.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use std::sync::Arc;

use ahash::{HashMap, HashSet};
use itertools::Itertools as _;

use re_string_interner::InternedString;
use re_types_core::SizeBytes;

Expand Down Expand Up @@ -258,6 +261,82 @@ impl EntityPath {
let first = entities.next().cloned().unwrap_or(Self::root());
entities.fold(first, |acc, e| acc.common_ancestor(e))
}

/// Returns short names for a collection of entities based on the last part(s), ensuring
/// uniqueness. Disambiguation is achieved by increasing the number of entity parts used.
///
/// Note: the result is undefined when the input contains duplicates.
pub fn short_names_with_disambiguation(
entities: impl IntoIterator<Item = Self>,
) -> HashMap<Self, String> {
struct ShortenedEntity {
entity: EntityPath,

/// How many parts (from the end) to use for the short name
num_part: usize,
}

impl ShortenedEntity {
fn ui_string(&self) -> String {
if self.entity.parts.is_empty() {
return "/".to_owned();
}

self.entity
.iter()
.rev()
.take(self.num_part)
.rev()
.map(|part| part.ui_string())
.join("/")
}
}

let mut str_to_entities: HashMap<String, ShortenedEntity> = HashMap::default();
let mut known_bad_labels: HashSet<String> = HashSet::default();

for entity in entities {
let mut shortened = ShortenedEntity {
entity,
num_part: 1,
};

loop {
let new_label = shortened.ui_string();

if str_to_entities.contains_key(&new_label) || known_bad_labels.contains(&new_label)
{
// we have a conflict so:
// - we fix the previously added entity by increasing its `num_part`
// - we increase the `num_part` of the current entity
// - we record this label as bad

known_bad_labels.insert(new_label.clone());

if let Some(mut existing_shortened) = str_to_entities.remove(&new_label) {
existing_shortened.num_part += 1;
str_to_entities.insert(existing_shortened.ui_string(), existing_shortened);
}

shortened.num_part += 1;
if shortened.ui_string() == new_label {
// we must have reached the root for this entity, so we bail out to avoid
// an infinite loop
break;
}
} else {
break;
}
}

str_to_entities.insert(shortened.ui_string(), shortened);
}

str_to_entities
.into_iter()
.map(|(str, entity)| (entity.entity, str))
.collect()
}
}

impl SizeBytes for EntityPath {
Expand Down Expand Up @@ -537,4 +616,38 @@ mod tests {
EntityPath::root()
);
}

#[test]
fn test_short_names_with_disambiguation() {
fn run_test(entities: &[(&str, &str)]) {
let paths = entities
.iter()
.map(|(entity, _)| EntityPath::from(*entity))
.collect_vec();
let result = EntityPath::short_names_with_disambiguation(paths.clone());

for (path, shortened) in paths.iter().zip(entities.iter().map(|e| e.1)) {
assert_eq!(result[path], shortened);
}
}

// --

run_test(&[("foo/bar", "bar"), ("qaz/bor", "bor")]);

run_test(&[
("hello/world", "world"),
("bim/foo/bar", "foo/bar"),
("bim/qaz/bar", "qaz/bar"),
("a/x/y/z", "a/x/y/z"),
("b/x/y/z", "b/x/y/z"),
("c/d/y/z", "d/y/z"),
]);

run_test(&[("/", "/"), ("/a", "a")]);

// degenerate cases
run_test(&[("/", "/"), ("/", "/")]);
run_test(&[("a/b", "a/b"), ("a/b", "a/b")]);
}
}
12 changes: 7 additions & 5 deletions crates/viewer/re_space_view_time_series/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ mod space_view_class;
mod util;

use re_log_types::EntityPath;
use re_types::{
components::{AggregationPolicy, MarkerShape},
datatypes::Utf8,
};
use re_types::components::{AggregationPolicy, MarkerShape};
pub use space_view_class::TimeSeriesSpaceView;

/// Computes a deterministic, globally unique ID for the plot based on the ID of the space view
Expand Down Expand Up @@ -76,7 +73,12 @@ pub enum PlotSeriesKind {

#[derive(Clone, Debug)]
pub struct PlotSeries {
pub label: Utf8,
/// 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 color: egui::Color32,

/// Radius of markers, or stroke radius for lines.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,7 @@ impl TypedComponentFallbackProvider<StrokeWidth> for SeriesLineSystem {
}
}

impl TypedComponentFallbackProvider<Name> for SeriesLineSystem {
fn fallback_for(&self, ctx: &QueryContext<'_>) -> Name {
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]);
re_viewer_context::impl_component_fallback_provider!(SeriesLineSystem => [Color, StrokeWidth]);

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

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

impl TypedComponentFallbackProvider<Name> for SeriesPointSystem {
fn fallback_for(&self, ctx: &QueryContext<'_>) -> Name {
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]);
re_viewer_context::impl_component_fallback_provider!(SeriesPointSystem => [Color, MarkerSize]);

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

// Now convert the `PlotPoints` into `Vec<PlotSeries>`
points_to_series(
Expand All @@ -459,7 +450,7 @@ impl SeriesPointSystem {
points,
ctx.recording_store(),
view_query,
&series_name,
series_name,
// Aggregation for points is not supported.
re_types::components::AggregationPolicy::Off,
all_series,
Expand Down
17 changes: 15 additions & 2 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 @@ -427,6 +427,12 @@ 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(
all_plot_series
.iter()
.map(|series| series.entity_path.clone()),
);

for series in all_plot_series {
let points = series
.points
Expand All @@ -447,17 +453,24 @@ 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(series.label.as_str())
.name(label)
.color(color)
.width(2.0 * series.radius_ui)
.id(id),
),
PlotSeriesKind::Scatter(scatter_attrs) => plot_ui.points(
Points::new(points)
.name(series.label.as_str())
.name(label)
.color(color)
.radius(series.radius_ui)
.shape(scatter_attrs.marker.into())
Expand Down
12 changes: 6 additions & 6 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_name: &re_types::components::Name,
series_label: Option<String>,
aggregator: AggregationPolicy,
all_series: &mut Vec<PlotSeries>,
) {
Expand All @@ -110,7 +110,7 @@ pub fn points_to_series(
}

all_series.push(PlotSeries {
label: series_name.0.clone(),
label: series_label,
color: points[0].attrs.color,
radius_ui: points[0].attrs.radius_ui,
kind,
Expand All @@ -122,7 +122,7 @@ pub fn points_to_series(
});
} else {
add_series_runs(
series_name,
&series_label,
points,
entity_path,
aggregator,
Expand Down Expand Up @@ -201,7 +201,7 @@ pub fn apply_aggregation(

#[inline(never)] // Better callstacks on crashes
fn add_series_runs(
series_name: &re_types::components::Name,
series_label: &Option<String>,
points: Vec<PlotPoint>,
entity_path: &EntityPath,
aggregator: AggregationPolicy,
Expand All @@ -214,7 +214,7 @@ fn add_series_runs(
let num_points = points.len();
let mut attrs = points[0].attrs.clone();
let mut series: PlotSeries = PlotSeries {
label: series_name.0.clone(),
label: series_label.clone(),
color: attrs.color,
radius_ui: attrs.radius_ui,
points: Vec::with_capacity(num_points),
Expand All @@ -238,7 +238,7 @@ fn add_series_runs(
let prev_series = std::mem::replace(
&mut series,
PlotSeries {
label: series_name.0.clone(),
label: series_label.clone(),
color: attrs.color,
radius_ui: attrs.radius_ui,
kind: attrs.kind,
Expand Down

0 comments on commit cd951dd

Please sign in to comment.