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.

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

don't request tiles outside the total profile interval #40

Merged

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Nov 14, 2023

This PR makes the view avoid requesting tiles outside the profile total_interval, which can otherwise lead to crashes.

fixes: #39

cc @elliottslaughter

src/app.rs Outdated
@@ -1322,7 +1322,8 @@ impl Config {
}
}

fn request_tiles(&mut self, request_interval: Interval) -> Vec<TileID> {
fn request_tiles(&mut self, cx: &mut Context) -> Vec<TileID> {
let request_interval = cx.view_interval.intersection(cx.total_interval);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want self.interval not cx.total_interval (and therefore we do not need to pass cx at all).

Copy link
Contributor Author

@bryevdv bryevdv Nov 17, 2023

Choose a reason for hiding this comment

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

cb0343a

BTW the viewer still crashes if you scroll far enough so that all the data is outside the view interval. But that points towards being defensive on the other end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. We should probably harden the StateDataSource to avoid crashing when the interval is empty. There are (at least in principle) reasons why you might want to do this when viewing multiple profiles. (E.g., you could be off the end of one profile but not of another.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can submit a PR to legion next week after this one is merged

@elliottslaughter elliottslaughter merged commit b630017 into StanfordLegion:master Nov 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on initial pan left after start
2 participants