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

TimeSeries y-range is now tightly synced with plot view & uses new generic ui #6485

Merged
merged 9 commits into from
Jun 4, 2024
Merged

TimeSeries y-range is now tightly synced with plot view & uses new generic ui #6485

merged 9 commits into from
Jun 4, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jun 3, 2024 •
edited
Loading

What

Easier demonstrated than described:
https://github.com/rerun-io/rerun/assets/1220815/11fb9b5a-3276-4391-9e7c-1b6b7fce2bd7

  • ⚠️ api breaking change: renamed parameter lock_range_during_zoom to zoom_lock
    • otherwise it's really clunky in the ui, which hints that this is too long to begin with!
  • y_range is now directly used to reflect whats in the time series plot
  • use new generic ui for all display & reset needs

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
  • If applicable, add a new check to the release checklist!

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🟦 blueprint The data that defines our UI include in changelog labels Jun 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 3, 2024 •
edited
Loading

Deployed docs

Commit Link
811e55c https://landing-89ecbgowf-rerun.vercel.app/docs

Base automatically changed from andreas/pass-view-state-to-visualizers to main June 3, 2024 14:38
@Wumpf Wumpf force-pushed the andreas/y-range-auto-ui branch from d16a39b to f32d1fb Compare June 3, 2024 14:39
@Wumpf Wumpf force-pushed the andreas/y-range-auto-ui branch from f32d1fb to 2f5c33f Compare June 3, 2024 16:22
@Wumpf Wumpf marked this pull request as draft June 3, 2024 17:41
@Wumpf Wumpf marked this pull request as draft June 3, 2024 17:41
@Wumpf
Copy link
Member Author

Wumpf commented Jun 3, 2024

drafted: @jleibs noticed a bug where we sometimes go to -1 to 1 because egui_plot isn't happy and write that to the blueprint then, causing us to loose the meaningful default that would come in later

@Wumpf Wumpf marked this pull request as ready for review June 4, 2024 09:22
@abey79 abey79 self-requested a review June 4, 2024 13:00
abey79
abey79 approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

Great stuff! Mostly perplexed by the "Scalar range" property name—see my comment.

crates/re_space_view_time_series/src/space_view_class.rs
Comment on lines +43 to +44
/// The range of the scalar values currently on screen.
scalar_range: Range1D,
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to state if this is the X or Y range.

Edit: looked at the video again, and now I finally understand that in this context scalar (axis) meaning is as opposed to temporal (axis). I got confused because the entire thing is also about scalars. Wonder if there is something to improve here? 🤔 Value range? Magnitude range?

Copy link
Member Author

@Wumpf Wumpf Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

uhm.... I don't disagree necessarily but while I don't feel strongly about the name itself, I do feel strongly about not wanting to open a naming discussion here about some decision someone made at some point.
Plz open an issue if you think this is worth refactoring/renaming. Thank you for your understanding 🙇

crates/re_space_view_time_series/src/space_view_class.rs
@@ -188,7 +181,7 @@ impl SpaceViewClass for TimeSeriesSpaceView {
);

view_property_ui::<PlotLegend>(ctx, ui, space_view_id, self, state);
axis_ui(ctx, space_view_id, ui, state);
view_property_ui::<ScalarAxis>(ctx, ui, space_view_id, self, state);
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@Wumpf Wumpf merged commit d7c7237 into main Jun 4, 2024
40 checks passed
@Wumpf Wumpf deleted the andreas/y-range-auto-ui branch June 4, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abey79 abey79 abey79 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.
2 participants
@Wumpf @abey79