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

Resolve unexpected view-partitioning by only bucket images when creating a new 2d view #4361

Merged
merged 1 commit into from Nov 29, 2023
Merged

Resolve unexpected view-partitioning by only bucket images when creating a new 2d view #4361

merged 1 commit into from Nov 29, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Nov 28, 2023 •
edited

What

Fixes two issues:

  • Image bucketing only makes sense for 2d spatial views
  • The bucket logic would previously trigger even when the aggregate view should have lost out according to the scoring, so we move this into the should_spawn_new code-path.

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 app.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@jleibs jleibs marked this pull request as ready for review November 28, 2023 14:08
@jleibs jleibs added ui concerns graphical user interface include in changelog labels Nov 28, 2023
@jleibs jleibs changed the title Only bucket images when creating a new 2d view Resolve unexpected view-partitioning by only bucket images when creating a new 2d view Nov 28, 2023
abey79
abey79 approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@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.

Nice!

crates/re_viewport/src/space_view_heuristics.rs
@@ -27,6 +27,10 @@ fn is_spatial_class(class: &SpaceViewClassName) -> bool {
class.as_str() == "3D" || class.as_str() == "2D"
}

fn is_spatial_2d_class(class: &SpaceViewClassName) -> bool {
class.as_str() == "2D"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class.as_str() == "2D"
class.as_str() == SpatialSpaceView2D::NAME

The function above should follow the same pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Relevant: #4386

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion decided not to make this change since it requires picking up an unwanted dependency -- will refactor away in #4388

@jleibs jleibs merged commit e0e22d4 into main Nov 29, 2023
39 of 40 checks passed
@jleibs jleibs deleted the jleibs/image_heuristics branch November 29, 2023 14:19
teh-cmc pushed a commit that referenced this pull request Nov 30, 2023
@jleibs @teh-cmc
Resolve unexpected view-partitioning by only bucket images when creat…
c2593b9 
…ing a new 2d view (#4361)

### What
- Resolves: #4284

Fixes two issues:
 - Image bucketing only makes sense for 2d spatial views
- The bucket logic would previously trigger even when the aggregate view
should have lost out according to the scoring, so we move this into the
`should_spawn_new` code-path.

### 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 [app.rerun.io](https://app.rerun.io/pr/4361) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4361)
- [Docs
preview](https://rerun.io/preview/c996f7d7a26f19ce3c281e25663d39ed8f8f4016/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/c996f7d7a26f19ce3c281e25663d39ed8f8f4016/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
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
include in changelog ui concerns graphical user interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Visualizer unexpectedly splits 3D space view into two sub-groups of images
2 participants
@jleibs @abey79