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.

Sign up for GitHub

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

Jump to bottom

Introduce Scalar, SeriesLine, and SeriesPoint archetypes with their own visualizers #4875

Merged
merged 37 commits into from
Feb 1, 2024
Merged

Introduce Scalar, SeriesLine, and SeriesPoint archetypes with their own visualizers #4875

merged 37 commits into from
Feb 1, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Jan 19, 2024 •
edited by github-actions bot
Loading

What

  • New Scalar, SeriesLine, and SeriesPoint types
  • Factor out common plot operations for reuse across visualizers
  • New visualizer systems for Line and Point
  • New component to control overriden visualizers
  • New UI to modify the component

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@jleibs jleibs added do-not-merge Do not merge this PR 🟦 blueprint The data that defines our UI labels Jan 19, 2024
@jleibs jleibs force-pushed the jleibs/series_style branch 2 times, most recently from 5b5146f to 7293db7 Compare January 29, 2024 17:48
@jleibs jleibs changed the title [WIP] Introduce SeriesStyle and a mechanism to override it from the selection panel Introduce SeriesStyle archetype Jan 29, 2024
@jleibs jleibs added include in changelog and removed do-not-merge Do not merge this PR labels Jan 29, 2024
@jleibs jleibs force-pushed the jleibs/series_style branch from 7293db7 to eb4bac8 Compare January 30, 2024 21:42
@jleibs jleibs changed the title Introduce SeriesStyle archetype WIP: Introduce Scalar, SeriesLine, and SeriesPoint archetypes with their own visualizers Jan 30, 2024
@jleibs jleibs force-pushed the jleibs/series_style branch from eb4bac8 to 20446a4 Compare January 31, 2024 23:44
jleibs
jleibs commented Feb 1, 2024
View reviewed changes
crates/re_space_view_time_series/src/aggregation.rs Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just moving all the aggregation pieces out of visualizer_system.

@Wumpf Wumpf self-requested a review February 1, 2024 14:56
Wumpf
Wumpf previously requested changes Feb 1, 2024
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

looking good overall. Heuristic needs patching and duplication of amount of aggregation is problematic

Was fairly trusting on the moved code, hard to follow all of the details how which thing moved

crates/re_types/definitions/rerun/archetypes/scalar.fbs Show resolved Hide resolved
crates/re_types/definitions/rerun/blueprint/archetypes/series_line.fbs Outdated Show resolved Hide resolved
crates/re_types/definitions/rerun/components/visualizer_overrides.fbs Show resolved Hide resolved
crates/re_space_view/src/data_query_blueprint.rs Show resolved Hide resolved
crates/re_space_view/src/data_query_blueprint.rs Outdated Show resolved Hide resolved
crates/re_space_view_time_series/src/space_view_class.rs Outdated Show resolved Hide resolved
crates/re_space_view_time_series/src/visualizer_system.rs
#[derive(Default, Debug)]
pub struct TimeSeriesSystem {
pub struct LegacyTimeSeriesSystem {
Copy link
Member

Choose a reason for hiding this comment

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

rename the file as well?

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 originally did but it made the git diff much more annoying because it failed to track the move

crates/re_space_view_time_series/src/visualizer_system.rs Show resolved Hide resolved
crates/re_space_view_time_series/src/lib.rs Outdated Show resolved Hide resolved
crates/re_space_view_time_series/src/line_visualizer_system.rs Outdated Show resolved Hide resolved
@jleibs jleibs dismissed Wumpf’s stale review February 1, 2024 19:42

All changes have been made.

@jleibs jleibs force-pushed the jleibs/series_style branch from 31e7e05 to caadc94 Compare February 1, 2024 19:49
@jleibs jleibs force-pushed the jleibs/series_style branch from caadc94 to ff87c9e Compare February 1, 2024 20:11
Wumpf
Wumpf approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

🚢

crates/re_space_view_time_series/src/space_view_class.rs
Comment on lines +219 to +223
for indicated in [
LegacyTimeSeriesSystem::identifier(),
SeriesLineSystem::identifier(),
SeriesPointSystem::identifier(),
]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm if one would only log the new scalar archetype, then this wouldn't spawn a space view. But then again we also don't know what visualizer to activate right now, so I figure this is correct? All a bit confusing with the Legacy one around right 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.

This actually works because the SeriesLineSystem and SeriesPointSystem both accept Scalar as an indicator.

tests/roundtrips.py
Comment on lines +31 to +33
"scalar": ["cpp", "py", "rust"], # TODO(jleibs)
"series_line": ["cpp", "py", "rust"], # TODO(jleibs)
"series_point": ["cpp", "py", "rust"], # TODO(jleibs)
Copy link
Member

Choose a reason for hiding this comment

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

do we really need roundtrip tests for these? worth bringing up as a post-standup topic how many of these we want/need in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, TODO there until I've at least made sure they are covered somehow. With it's implicit via code-examples or explicit via RTT.

@jleibs jleibs merged commit 0db7a05 into main Feb 1, 2024
43 checks passed
@jleibs jleibs deleted the jleibs/series_style branch February 1, 2024 20:37
@abey79 abey79 added ui concerns graphical user interface 📺 re_viewer affects re_viewer itself and removed ui concerns graphical user interface labels Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
🟦 blueprint The data that defines our UI include in changelog 📺 re_viewer affects re_viewer itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jleibs @Wumpf @abey79 @teh-cmc