-
Notifications
You must be signed in to change notification settings - Fork 373
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
Fix presence of segmentation images and tensor causing generation of root space view #3681
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is_interesting_space_view_at_root
used to return true
for segmentation images, but I don't understand how that would put it in the same spaceview as an entity with a different root?
// If there are any images directly under the root, don't create root space either. | ||
// -> For images we want more fine grained control and resort to child-of-root spaces only. | ||
for entity_path in &candidate.contents.root_group().entities { | ||
if contains_any_image(entity_path, data_store) { | ||
if contains_tensor_data(entity_path, data_store) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this mess things up for non-2D tensors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. The old code did this:
https://github.com/rerun-io/rerun/blob/release-0.8/crates/re_viewport/src/space_view_heuristics.rs#L76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned now. I could do the 2dness check on top and be mostly with the old behavior but this has slight regression potential on the fix from #3583
Well, let's try it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it would be probably best then to check for Image and SegmentationImage indicators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. This seems safest, easiest and passes both repro scripts
Root is the root here, i.e. |
|
What
Definitely need to come up with something better to steer the heuristics - we made good strides in space view refactor and recent advances in System heuristics, but there's still a lot to be desired!
In the test scene there's still the issue of a unexplicably large accumulated bounding box which causes the camera to zoom out a lot. This is most likely caused by an initially bad heuristic for the frustum length. I suspect this is a problem we already had in 0.8 and might be fairly hard to solve until we fixup our blueprint & object property story.
Before:
After:
Test code:
Also made sure this doesn't regress the fix in #3583 (which touched this code last!)
Checklist