Skip to content

Configurable dynamic plot aggregation based on zoom-level #4865

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
Merged
teh-cmc merged 8 commits into from
Jan 25, 2024
Merged

Configurable dynamic plot aggregation based on zoom-level #4865

teh-cmc merged 8 commits into from
Jan 25, 2024

Conversation

@teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 18, 2024 •
edited by github-actions bot
Loading

⚠️ Try it live! ⚠️

Make it so users can configure an aggregation strategy in the rare case where they either have so much data or are so zoomed out that most of their plot results in an overdraw blurb.

Because this builds on top of the range cache, the data is neatly laid out in a memory slice already so this is very cheap to compute.

In my tests, the MinMax strategy has worked so well that I've decided to make it the default in the end... That might be controversial 😶.

Off vs. MinMax, using the new gaussian walk benchmark:
image (26)
image (27)

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

@teh-cmc teh-cmc added ui concerns graphical user interface 📺 re_viewer affects re_viewer itself 📉 performance Optimization, memory use, etc do-not-merge Do not merge this PR include in changelog labels Jan 18, 2024
teh-cmc
teh-cmc commented Jan 18, 2024
View reviewed changes
@teh-cmc teh-cmc force-pushed the cmc/aggregation_poc branch from d17bb54 to 75ff4ff Compare January 18, 2024 16:24
@nikolausWest
Copy link
Member

nikolausWest commented Jan 22, 2024

I couldn't really spot the differences in the video. Btw, what does MinMax actually do? I would have assumed it output two values, a min and a max.

@nikolausWest
Copy link
Member

nikolausWest commented Jan 22, 2024

As we present this option to users (both in the SDK and in the UI) I think it's important we clarify that this is about aggregating when we have multiple points per pixel in the plot

@emilk emilk self-requested a review January 23, 2024 07:51
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 23, 2024 •
edited
Loading

I couldn't really spot the differences in the video. Btw, what does MinMax actually do? I would have assumed it output two values, a min and a max.

It's exactly what it does, and that results in a line that goes straight up between those two points, since they both live in the same "pixel column".
The result is effectively indistinguishable from what the non-aggregated path would do in many cases, except we get there without any overdraw.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 23, 2024

As we present this option to users (both in the SDK and in the UI) I think it's important we clarify that this is about aggregating when we have multiple points per pixel in the plot

This is what the hover says at the moment:
image

emilk
emilk requested changes Jan 23, 2024
View reviewed changes
@teh-cmc teh-cmc force-pushed the cmc/primcache_19_statification branch from dd80167 to 5b4e1d4 Compare January 23, 2024 16:51
Base automatically changed from cmc/primcache_19_statification to main January 23, 2024 17:01
@teh-cmc teh-cmc force-pushed the cmc/aggregation_poc branch 2 times, most recently from 205d5e3 to 6e2eb83 Compare January 24, 2024 11:12
@github-actions
Copy link

github-actions bot commented Jan 24, 2024

Size changes

Name main 4865/merge Change
plots.rrd 204.80 kiB 194.56 kiB -5.00%

@teh-cmc teh-cmc marked this pull request as draft January 24, 2024 17:57
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Jan 25, 2024
@teh-cmc teh-cmc marked this pull request as ready for review January 25, 2024 14:40
@teh-cmc teh-cmc requested a review from emilk January 25, 2024 14:40
@nikolausWest
Copy link
Member

nikolausWest commented Jan 25, 2024

This is pretty sweet!

@teh-cmc teh-cmc force-pushed the cmc/aggregation_poc branch from a01bd3c to 633b697 Compare January 25, 2024 15:33
emilk
emilk approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Awesome ⭐

crates/re_space_view_time_series/src/lib.rs Outdated
/// ```
#[inline]
pub(crate) fn plot_id(space_view_id: re_viewer_context::SpaceViewId) -> egui::Id {
egui::Id::new(format!("plot_{space_view_id}"))
Copy link
Member

@emilk emilk Jan 25, 2024

Choose a reason for hiding this comment

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

faster, less hacky

Suggested change
egui::Id::new(format!("plot_{space_view_id}"))
egui::Id::new(("plot", space_view_id))

teh-cmc
teh-cmc commented Jan 25, 2024
View reviewed changes
crates/re_space_view_time_series/src/visualizer_system.rs Outdated
scattered,
} = attrs;

// We cannot aggregate two points that doesn't live in the same aggregation window to start with.
Copy link
Member Author

@teh-cmc teh-cmc Jan 25, 2024

Choose a reason for hiding this comment

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

don't

@teh-cmc teh-cmc force-pushed the cmc/aggregation_poc branch from 67a7767 to 3f868a4 Compare January 25, 2024 17:11
@teh-cmc teh-cmc merged commit 027bbe1 into main Jan 25, 2024
@teh-cmc teh-cmc deleted the cmc/aggregation_poc branch January 25, 2024 17:55
@emilk emilk mentioned this pull request Jan 26, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikolausWest nikolausWest nikolausWest left review comments

@emilk emilk emilk approved these changes

Assignees

No one assigned

Labels

include in changelog 📉 performance Optimization, memory use, etc 📺 re_viewer affects re_viewer itself ui concerns graphical user interface

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Configurable aggregation behaviors for time series plots

4 participants

@teh-cmc @nikolausWest @emilk